dimas-b commented on code in PR #4281:
URL: https://github.com/apache/polaris/pull/4281#discussion_r3356912094
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -95,8 +97,39 @@ protected JdbcMetaStoreManagerFactory() {}
@Produces
@ApplicationScoped
static DatasourceOperations produceDatasourceOperations(
- Instance<DataSource> dataSource, RelationalJdbcConfiguration
relationalJdbcConfiguration) {
- return new DatasourceOperations(dataSource.get(),
relationalJdbcConfiguration);
+ Instance<DataSource> defaultDataSource,
+ @Any Instance<DataSource> dataSources,
+ RelationalJdbcConfiguration relationalJdbcConfiguration) {
+ Optional<String> configuredType =
relationalJdbcConfiguration.databaseType();
+
+ // MySQL is the only backend that requires a dedicated named DataSource
(it needs the GPL
+ // MySQL driver, which is opt-in at build time and inactive by default).
When MySQL is
+ // explicitly configured but its named DataSource is not resolvable, fail
fast with actionable
+ // guidance instead of silently falling back to the default
(PostgreSQL-driver) DataSource —
+ // that fallback would otherwise surface much later as a confusing
database-type mismatch.
+ String mysql = DatabaseType.MYSQL.getDisplayName();
+ if (configuredType.map(type ->
type.equalsIgnoreCase(mysql)).orElse(false)) {
+ Instance<DataSource> mysqlDataSource = dataSources.select(new
DataSourceLiteral(mysql));
Review Comment:
Subsequently, if we use `configuredType` as the selector in all cases, the
special case-insensitive comparison may no longer be so necessary.. WDYT?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -95,8 +97,39 @@ protected JdbcMetaStoreManagerFactory() {}
@Produces
@ApplicationScoped
static DatasourceOperations produceDatasourceOperations(
- Instance<DataSource> dataSource, RelationalJdbcConfiguration
relationalJdbcConfiguration) {
- return new DatasourceOperations(dataSource.get(),
relationalJdbcConfiguration);
+ Instance<DataSource> defaultDataSource,
+ @Any Instance<DataSource> dataSources,
+ RelationalJdbcConfiguration relationalJdbcConfiguration) {
+ Optional<String> configuredType =
relationalJdbcConfiguration.databaseType();
+
+ // MySQL is the only backend that requires a dedicated named DataSource
(it needs the GPL
+ // MySQL driver, which is opt-in at build time and inactive by default).
When MySQL is
+ // explicitly configured but its named DataSource is not resolvable, fail
fast with actionable
+ // guidance instead of silently falling back to the default
(PostgreSQL-driver) DataSource —
+ // that fallback would otherwise surface much later as a confusing
database-type mismatch.
+ String mysql = DatabaseType.MYSQL.getDisplayName();
+ if (configuredType.map(type ->
type.equalsIgnoreCase(mysql)).orElse(false)) {
+ Instance<DataSource> mysqlDataSource = dataSources.select(new
DataSourceLiteral(mysql));
Review Comment:
nit: for non-MySQL datasources we select by the exact `configuredType`
string, but here we do a case-insensitive match and use the
`DatabaseType.MYSQL` name as the selector... why the difference?
To be clear: the case-insensitive match is fine, from my POV, but should we
not use the exact `configuredType` string as the selector?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -95,8 +97,39 @@ protected JdbcMetaStoreManagerFactory() {}
@Produces
@ApplicationScoped
static DatasourceOperations produceDatasourceOperations(
- Instance<DataSource> dataSource, RelationalJdbcConfiguration
relationalJdbcConfiguration) {
- return new DatasourceOperations(dataSource.get(),
relationalJdbcConfiguration);
+ Instance<DataSource> defaultDataSource,
+ @Any Instance<DataSource> dataSources,
+ RelationalJdbcConfiguration relationalJdbcConfiguration) {
+ Optional<String> configuredType =
relationalJdbcConfiguration.databaseType();
+
+ // MySQL is the only backend that requires a dedicated named DataSource
(it needs the GPL
+ // MySQL driver, which is opt-in at build time and inactive by default).
When MySQL is
+ // explicitly configured but its named DataSource is not resolvable, fail
fast with actionable
+ // guidance instead of silently falling back to the default
(PostgreSQL-driver) DataSource —
+ // that fallback would otherwise surface much later as a confusing
database-type mismatch.
+ String mysql = DatabaseType.MYSQL.getDisplayName();
+ if (configuredType.map(type ->
type.equalsIgnoreCase(mysql)).orElse(false)) {
+ Instance<DataSource> mysqlDataSource = dataSources.select(new
DataSourceLiteral(mysql));
+ if (!mysqlDataSource.isResolvable()) {
+ throw new IllegalStateException(
Review Comment:
Could we do this when we fall back to the default DataSource (line 131)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
