Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuring the Narayana transaction log to be stored in a Datasource #31066

Closed
Sanne opened this issue Feb 10, 2023 · 24 comments
Closed
Labels
area/narayana Transactions / Narayana kind/enhancement New feature or request

Comments

@Sanne
Copy link
Member

Sanne commented Feb 10, 2023

Description

The Narayana project has the capability to store the transaction logs into a JDBC Datasource; this should be our recommendation for users needing transaction recovery capabilities, especially when running in volatile containers.

We currently don't seemt to expose the configuration properties to enable this component and bind it to a Datasource managed by Quarkus; also Narayana is only exposing an API which accepts a JDBC URL while we would highly prefer to inject the Datasource by instance. Allowing the Datasource to be injected is most likely trivial but will need to be patched at the Narayana side first: we don't want to start additional connections which could bypass the connection pool limits and risk resource starvation.

Secondarily, schema creation control would need to be configurable. Currently Narayana will always check for the existence of the expected tables and attempt to create them when missing; it would be preferrable to expose some more flexibility, such as to log/generate the SQL scripts to create them instead, to allow controlling such aspects with schema evolution tools and/or potentially with different user credentials than the ones used at runtime.

@Sanne Sanne added kind/enhancement New feature or request area/narayana Transactions / Narayana labels Feb 10, 2023
@Sanne
Copy link
Member Author

Sanne commented Feb 10, 2023

P.S. obviously when enabling this capability, we'll need to require assignment of a Transactionmanager node identifier; ideally this could be set by an operator, but that's hypothetical: we don't want to require such an operator.

@mmusgrov
Copy link
Contributor

I've made the appropriate changes to Narayana but I now need to update the narayana-jta extension so that it can look at the config and, if required, create a DataSource (and pass it to the Narayana for use with the JDBCStore). Are there any examples of how I should do this, Sanne said that creating datasources directly is an "anti-pattern" and we should be using some other mechanism for it. Can someone, such as @Sanne for example, advise on how best to proceed, thanks.

@Sanne
Copy link
Member Author

Sanne commented Feb 27, 2023

@gsmet or @yrodiere can either of you help?

Narayana needs a reference to a datasource to be used to store the transaction logs.

I expect it to grab a reference to the default datasource when there's a single one or no configuration is provided, or it to allow being pointed to a particular datasource by name - very similar to other components.

@yrodiere
Copy link
Member

@mmusgrov I concur: you should have a configuration property in your extension that the user can set to point to a particular datasource, and if relevant use the default datasource.

The Hibernate ORM extension does something similar here:

private static Optional<JdbcDataSourceBuildItem> findJdbcDataSource(String persistenceUnitName,
HibernateOrmConfigPersistenceUnit persistenceUnitConfig, List<JdbcDataSourceBuildItem> jdbcDataSources) {
if (persistenceUnitConfig.datasource.isPresent()) {
return Optional.of(jdbcDataSources.stream()
.filter(i -> persistenceUnitConfig.datasource.get().equals(i.getName()))
.findFirst()
.orElseThrow(() -> new ConfigurationException(String.format(Locale.ROOT,
"The datasource '%1$s' is not configured but the persistence unit '%2$s' uses it."
+ " To solve this, configure datasource '%1$s'."
+ " Refer to https://quarkus.io/guides/datasource for guidance.",
persistenceUnitConfig.datasource.get(), persistenceUnitName))));
} else if (PersistenceUnitUtil.isDefaultPersistenceUnit(persistenceUnitName)) {
return jdbcDataSources.stream()
.filter(i -> i.isDefault())
.findFirst();
} else {
// if it's not the default persistence unit, we mandate an explicit datasource to prevent common errors
return Optional.empty();
}
}

In your case, I guess you will have to:

  • Add a dependency to io.quarkus:quarkus-agroal-spi to your POM
  • Add a build-time configuration class (e.g. NarayanaConfig) to your extension (see https://quarkus.io/guides/writing-extensions#configuration if you're unfamiliar with that)
  • Inject that config into your build by adding a parameter of type NarayanaConfig to NarayanaJtaProcessor#build.
  • Inject configured datasources into your build by adding a parameter List<JdbcDataSourceBuildItem> jdbcDataSources to NarayanaJtaProcessor#build.
  • Process the config and existing datasources, e.g. like this:
JdbcDataSourceBuildItem pickDataSource(NarayanaConfig config, List<JdbcDataSourceBuildItem> jdbcDataSources) {
        if (config.datasource().isEmpty()) {
            return jdbcDataSources.stream()
                    .filter(i -> i.isDefault())
                    .findFirst()
                    .orElseThrow(() -> new ConfigurationException(
                            "The Narayana JTA extension does not have a datasource configured,"
                                    + " so it defaults to the default datasource,"
                                    + " but that datasource is not configured."
                                    + " To solve this, either configure the default datasource,"
                                    + " referring to https://quarkus.io/guides/datasource for guidance,"
                                    + " or configure the datasource to use in the Narayana JTA extension "
                                    + " by setting property 'quarkus.jta.datasource' to the name of a configured datasource."));
          
        } else {
            return jdbcDataSources.stream()
                    .filter(i -> persistenceUnitConfig.datasource.get().equals(i.getName()))
                    .findFirst()
                    .orElseThrow(() -> new ConfigurationException(String.format(Locale.ROOT,
                            "The Narayana JTA extension is configured to use datasource '%1$s',"
                                    + " but that datasource is not configured."
                                    + " To solve this, either configure datasource '%1$s',"
                                    + " referring to https://quarkus.io/guides/datasource for guidance,"
                                    + " or configure another datasource to use in the Narayana JTA extension "
                                    + " by setting property 'quarkus.jta.datasource' to the name of a configured datasource."
                            persistenceUnitConfig.datasource.get(), persistenceUnitName)));
        }
}
  • Use JdbcDataSourceBuildItem#getName() to retrieve the datasource name
  • At runtime (e.g. in a recorder), use Arc.container().instance(AgroalDataSource.class, new DataSource.DataSourceLiteral(datasourceName)).get() to retrieve the datasource.

Don't hesitate to ping me if that's not enough to get you going; perhaps it would be easier to discuss this on Zulip.

@zhfeng
Copy link
Contributor

zhfeng commented Mar 7, 2023

@mmusgrov @Sanne if I understand correctly, I suppose that quarkus.transaction.object-store-datasource is optional which means if it is not present, TransactionManager should use objectStore in file system but not a default datasource. Is it right?

Otherwise, we need to introduce a quarkus.transaction.object-store-datasource.enabled to set it true for using a datasource as objectStore.

@mmusgrov
Copy link
Contributor

mmusgrov commented Mar 7, 2023

We will be providing an option to configure a datasource for the JDBC Store so yes, if the user configures one then it will used otherwise we will still fallback to using the file system.

@yrodiere thanks for your detailed explanation of what I need to do but, to be honest I'm not that familiar with this kind of set up, for example what's the relationship between the NarayanaConfig/application.properties and the JdbcDataSourceBuildItem and how would the user define which datasource they want, perhaps their is an example that is a bit closer to our requirement.

@yrodiere
Copy link
Member

yrodiere commented Mar 7, 2023

what's the relationship between the NarayanaConfig/application.properties and the JdbcDataSourceBuildItem

None whatsoever, at the moment. You will introduce the relationship: users put a datasource name in NarayanaConfig/application.properties, and you look for that name in the list of datasources; each datasource is represented as a JdbcDataSourceBuildItem.

how would the user define which datasource they want

In application.properties:

quarkus.datasource."my-datasource".db-kind=postgresql
quarkus.datasource."my-datasource".username=sarah
quarkus.datasource."my-datasource".password=connor
quarkus.datasource."my-datasource".jdbc.url=jdbc:postgresql://localhost:5432/mydatabase

quarkus.transaction.object-store.datasource = my-datasource
# Or quarkus.jta, or quarkus.narayana-jta, or quarkus.narayana, ... pick your prefix.

That being said, the default datasource doesn't have a name, so if you want the selection of a datasource to be explicit, this won't work:

# this configures the default datasource
quarkus.datasource.db-kind=postgresql
quarkus.datasource.username=sarah
quarkus.datasource.password=connor
quarkus.datasource.jdbc.url=jdbc:postgresql://localhost:5432/mydatabase

quarkus.transaction.object-store.datasource = # what do I put here?

So it would indeed be a good idea to introduce a switch that doesn't involve selecting a datasource:

# this configures the default datasource
quarkus.datasource.db-kind=postgresql
quarkus.datasource.username=sarah
quarkus.datasource.password=connor
quarkus.datasource.jdbc.url=jdbc:postgresql://localhost:5432/mydatabase

# This will lead Narayana to use JDBC for the object store
# If `quarkus.transaction.object-store.datasource` is set, it will use that datasource
# Otherwise it will use the default datasource (if any)
quarkus.transaction.object-store.type = jdbc

perhaps their is an example that is a bit closer to our requirement.

There are, but they are more complex than what you need, so... If you're ready for some digging, see:

/**
* The name of the datasource which this persistence unit uses.
* <p>
* If undefined, it will use the default datasource.
*/
@ConvertWith(TrimmedStringConverter.class)
public Optional<String> datasource;

private static Optional<JdbcDataSourceBuildItem> findJdbcDataSource(String persistenceUnitName,
HibernateOrmConfigPersistenceUnit persistenceUnitConfig, List<JdbcDataSourceBuildItem> jdbcDataSources) {
if (persistenceUnitConfig.datasource.isPresent()) {
return Optional.of(jdbcDataSources.stream()
.filter(i -> persistenceUnitConfig.datasource.get().equals(i.getName()))
.findFirst()
.orElseThrow(() -> new ConfigurationException(String.format(Locale.ROOT,
"The datasource '%1$s' is not configured but the persistence unit '%2$s' uses it."
+ " To solve this, configure datasource '%1$s'."
+ " Refer to https://quarkus.io/guides/datasource for guidance.",
persistenceUnitConfig.datasource.get(), persistenceUnitName))));
} else if (PersistenceUnitUtil.isDefaultPersistenceUnit(persistenceUnitName)) {
return jdbcDataSources.stream()
.filter(i -> i.isDefault())
.findFirst();
} else {
// if it's not the default persistence unit, we mandate an explicit datasource to prevent common errors
return Optional.empty();
}
}

`

@zhfeng
Copy link
Contributor

zhfeng commented Mar 7, 2023

That being said, the default datasource doesn't have a name, so if you want the selection of a datasource to be explicit, this won't work:

IIRC, the default datasource name would be <default>. see

public static final String DEFAULT_DATASOURCE_NAME = "<default>";
public static boolean isDefault(String dataSourceName) {
return DEFAULT_DATASOURCE_NAME.equals(dataSourceName);
}

But I agree that quarkus.transaction.object-store.type is a good idea. It could be an enum value with FILE, JDBC, etc or something we will support in the future.

@mmusgrov I can help you to setup these ConfigItem.

@mmusgrov
Copy link
Contributor

mmusgrov commented Mar 7, 2023

@zhfeng That is a most kind offer, thank you Amos.

And I have a question; if the datasource needs to be defined in the application properties file wouldn't that mean the user wants to use the JDBCStore in which case requiring him to also define a store type might introduce ambiguity. That said, there are two file system store types (the default ShadowingStore one and the journal based one) so we also need a way to disambiguate those two alternatives.

@yrodiere
Copy link
Member

yrodiere commented Mar 7, 2023

And I have a question; if the datasource needs to be defined in the application properties file wouldn't that mean the user wants to use the JDBCStore in which case requiring him to also define a store type might introduce ambiguity. That said, there are two file system store types (the default ShadowingStore one and the journal based one) so we also need a way to disambiguate those two alternatives.

You can implement whatever behavior you want:

  • Decide on a hardcoded default type and use that
  • Make the default type dynamic based on settings: if quarkus.transaction.object-store.datasource is set, default to type=jdbc, otherwise use some other filesystem-based type
  • Or even make the default type dynamic based on application dependencies (more complex to implement, but doable)
  • ...

When implementing such dynamic defaults, the convention is general to define the corresponding config property as an Optional<...>, so that you can detect that the user didn't set anything. From there you write your own code to decide on the dynamic default.

@zhfeng
Copy link
Contributor

zhfeng commented Mar 8, 2023

Thanks @yrodiere and I prefer to the simple one with hardcodes default type at this time.
The defaut type should be FileSystem, and if user set it to JDBC explictly and no quarkus.transaction.object-store.datasource, we will try to use default datasource which could be similar process with hibernate example.

@mmusgrov WDYT?

@Sanne do we need to backport this one to 2.16.x ?

@Sanne
Copy link
Member Author

Sanne commented Mar 8, 2023

@zhfeng no I wouldn't backport at this stage. We can certainly take that in consideration in the future if there's need for it, but let's give it some time first.

@zhfeng
Copy link
Contributor

zhfeng commented Mar 9, 2023

@mmusgrov @yrodiere Please take a review of above PR!

  • Add quarkus.transaction.object-store.datasource ConfigItem
  • Get DataSource from JdbcDataSourceBuildItem

@zhfeng
Copy link
Contributor

zhfeng commented Mar 10, 2023

Unfortunately there is cycle dependent between agroal and narayana-jta extension currently

[INFO] Running io.quarkus.agroal.test.AgroalDevModeTestCase
[INFO] H2 database started in TCP server mode; server status: TCP server running at tcp://10.1.110.0:9092 (only local connections)
Error:  Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 6.269 s <<< FAILURE! - in io.quarkus.agroal.test.AgroalDevModeTestCase
Error:  io.quarkus.agroal.test.AgroalDevModeTestCase.testAgroalHotReplacement  Time elapsed: 6.204 s  <<< ERROR!
java.lang.RuntimeException: 
io.quarkus.builder.ChainBuildException: Cycle detected:
		   io.quarkus.narayana.jta.deployment.NarayanaJtaProcessor#build produced class io.quarkus.narayana.jta.deployment.NarayanaInitBuildItem
		to io.quarkus.agroal.deployment.AgroalProcessor#generateDataSourceBeans produced class io.quarkus.agroal.spi.JdbcDataSourceBuildItem
		to io.quarkus.narayana.jta.deployment.NarayanaJtaProcessor#build

agroal produces JdbcDataSourceBuildItem and needs XAResourceRecoveryRegistry to register the XAResourceRecovery. In the same time, XAResourceRecoveryRegistry loads the XARecoveryModule which uses StoreManager.getRecoveryStore() to start a object store. Then if we set to use JDBCStore, it needs a DataSource from JdbcDataSourceBuildItem where it is a cycle.

So we need to have some changes

  • For quarkus-narayana-jta, create a new BuildStep to config the object store after agroal produces JdbcDataSourceBuildItem
  • For narayana, change to lazy get recovery store so don't do this in constructor but in the first time using it.

@zhfeng
Copy link
Contributor

zhfeng commented Mar 10, 2023

@mmusgrov I'm not sure it is feasible for narayana with these changes. We need a further discussion

@mmusgrov
Copy link
Contributor

I don't know how to break these cycles so we need to take advice from someone in the Quarkus team, @Sanne or @yrodiere can you suggest someone that could help us break this cycle reported by the io.quarkus.builder?

@yrodiere
Copy link
Member

, @Sanne or @yrodiere can you suggest someone that could help us break this cycle reported by the io.quarkus.builder?

From what I understand, the cycle is caused by the (artificial) dependency of Agroal on @Consume(NarayanaInitBuildItem.class), combined the dependency of Narayana to the list of datasources.

I'd need to understand why that artificial dependency (@Consume(NarayanaInitBuildItem.class)) was introduced.

If it's only to ensure things happen in the correct order during static/runtime init (i.e. in recorders), then we may have other solutions than just introducing dependencies between build items.

If it's actually some build-time dependency, we might be able to solve the problem by splitting io.quarkus.narayana.jta.deployment.NarayanaJtaProcessor#build into multiple steps, with one producing NarayanaInitBuildItem and another (unrelated) one consuming JdbcDataSourceBuildItem.

Can someone explain what the @Consume(NarayanaInitBuildItem.class) achieves exactly?

@mmusgrov
Copy link
Contributor

That dependency was introduced here, @zhfeng is the @Consume annotation definitely required?

@yrodiere
Copy link
Member

I suppose that was the explanation:

agroal produces JdbcDataSourceBuildItem and needs XAResourceRecoveryRegistry to register the XAResourceRecovery. In the same time, XAResourceRecoveryRegistry loads the XARecoveryModule which uses StoreManager.getRecoveryStore() to start a object store. Then if we set to use JDBCStore, it needs a DataSource from JdbcDataSourceBuildItem where it is a cycle.

Though I'm not sure if which of those are build-time dependencies and which are runtime dependencies.

If it's exclusively a runtime problem (e.g. Datasource needs XARecoveryModule to start and vice-versa) we might need to introduce some lazy initialization here and there, but that would probably involve changes outside of Quarkus.

@mmusgrov
Copy link
Contributor

We already need a release of Narayana to facilitate this work so it's no problem adding more changes to Narayana.

@zhfeng
Copy link
Contributor

zhfeng commented Mar 10, 2023

Yeah, that's exactly my proposal to introduce a lazy initialization of RecoveryStore in Narayana. But I think these codes are critical in recovery process and we have to take a look very carefully.

@zhfeng
Copy link
Contributor

zhfeng commented Mar 13, 2023

Now I'm blocked by JBTM-3760 to test my idea with the Narayana 6.0.1-SNAPSHOT.

@michalvavrik
Copy link
Member

Is this fixed with #31729?

@yrodiere
Copy link
Member

yrodiere commented Jul 9, 2023

It is, thanks for the heads-up.

@yrodiere yrodiere closed this as completed Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/narayana Transactions / Narayana kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants