You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "singhpk234 (via GitHub)" <gi...@apache.org> on 2025/04/28 16:00:19 UTC
[PR] [WIP] Support 1 DS per realm [polaris]
singhpk234 opened a new pull request, #1482:
URL: https://github.com/apache/polaris/pull/1482
### About the change
NOTE: Not ready for complete review.
This introduces a new config `polaris.persistence.realm.isolation.type` which when set to db, which would imply that there is 1 DB per realm.
Presently this just makes the test pass of Admin and i am putting this out to get some early feedback.
This also add a check for table not exists when doing a check if the realm is bootstrapped or not so that purge can proceed, as if the realm is not bootstraped tables itself will not be there.
```
if (e.getSQLState().equals("42P01")) {
return null;
}
```
I observed some unexplainable quarkus behaviour and spent days on debugging it, but now i have some way to work around, with custom DS of quarkus if i don't specify them in application prop and even though i specify it my PostgresLifeCycleManagement test container it doesn't create these beans, so declearing even the garbage in the application prop does the trick.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067452821
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
Use explicit default DS configuration makes sense to me.
I'd also suggest setting built-in configuration such that out-of-the-box all realms map to the "default" datasource (for simplicity), but I do not feel too strongly about this.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069531130
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
If `quarkus.datasource.\"realm1_ds\".db-kind=pgsql` has to be defined at build time, that would be a problem for user of the binary distribution, I guess.
I'm surprised this is required, because the default datasource does not have to be defined at build time, if I'm not mistaken.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069548146
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
> If quarkus.datasource.\"realm1_ds\".db-kind=pgsql has to be defined at build time, that will be a problem for users of the binary distribution, I guess.
See https://quarkus.io/guides/datasource#configure-multiple-datasources:
> Even when only one database extension is installed, named databases need to specify at least one build-time property so that Quarkus can detect them. Generally, this is the db-kind property [...]
This is why I asked this question, I think there is some misunderstanding going on: https://github.com/apache/polaris/pull/1482#issuecomment-2843429552
> I'm surprised this is required, because the default datasource does not have to be defined at build time, if I'm not mistaken.
The default datasource is treated slightly different: if `db-kind` is missing, the JDBC driver will be guessed from the available JDBC drivers.
But that doesn't help here, since the intent is to use many named datasources.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069672287
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
from my POV, it might be fine to support one Data Source (co-located realms) out-of-the-box and instruct users to use custom builds if they want to segregate realms by Data Source (similar experience to EclipseLink).
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
from my POV, it might be fine to support one Data Source (co-located realms) out-of-the-box and instruct users to use custom builds if they want to segregate realms by Data Source (experience similar to EclipseLink).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069488866
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
The problem here is that for a Datasource to be created by Quarkus, there has to be at least one build-time property defined for that datasource: `db-kind`.
And you cannot pass build-time properties through `QuarkusTestResourceLifecycleManager#start`.
So `PostgresRelationalJdbcLifeCycleManagement` is trying to create datasources dynamically, but since this is happening at runtime, this is not working.
Thus for tests you _have_ to declare the datasources you intend to use here, along with their db-kind:
```properties
quarkus.datasource.\"realm1_ds\".db-kind=pgsql
quarkus.datasource.\"realm2_ds\".db-kind=pgsql
quarkus.datasource.\"realm3_ds\".db-kind=pgsql
# etc
```
Other runtime properties, like `jdbc.url`, `username` and `password` can (and should) be defined in `PostgresRelationalJdbcLifeCycleManagement`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069672287
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
from my POV, it might be fine to support one Data Source (co-located realms) out-of-the-box and instruct users to use custom builds if they want to segregate realms by Data Source.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069668972
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
It would be ideal to let Quarkus manage the Data Sources, but having to enumerate them at build-time kills the user-level benefits, IMHO.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069014274
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.relational.jdbc;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Map;
+
+@ConfigMapping(prefix = "polaris.relation.jdbc")
+public interface RelationalJdbcConfiguration {
Review Comment:
I moved this polaris-core under config, let me know how do you feel about it, since we already take a dependency on core in the quarkus/common this sub-interface would not be required and hence wired this directly to the QuarkusProducers.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #1482:
URL: https://github.com/apache/polaris/pull/1482#issuecomment-2843497253
> realm-1, realm-2 we go to dsA and dsB both need to predefined
OK then, that is possible 👍 I was worried you would be wanting that realm-3 should trigger the creation of an extra datasource.
Thanks for confirming.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2065412396
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusDatasourceSupplier.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.admintool.config;
+
+import io.quarkus.arc.InstanceHandle;
+import java.util.List;
+import javax.sql.DataSource;
+import org.apache.polaris.extension.persistence.relational.jdbc.DatasourceSupplier;
+import org.apache.polaris.extension.persistence.relational.jdbc.RelationalJdbcConfiguration;
+
+public class QuarkusDatasourceSupplier implements DatasourceSupplier {
Review Comment:
I know this is a copy of the code in the service, but getting this in the commons causes a depedency hell for me. I am working on fixing this.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2078350791
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
Why not configure those DS using a quarkus-test-profile?
##########
quarkus/common/src/main/java/org/apache/polaris/common/QuarkusDatasourceSupplier.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.common;
Review Comment:
Common for everything?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2078456335
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
sounds fair ! its on my list, should be sending out an email soon, meanwhile marking this PR as draft to not draw more attention,
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067488312
##########
extension/persistence/relational-jdbc/build.gradle.kts:
##########
@@ -32,6 +32,7 @@ dependencies {
compileOnly(libs.jakarta.inject.api)
implementation(libs.smallrye.common.annotation) // @Identifier
+ compileOnly(libs.smallrye.config.core)
Review Comment:
for the ConfigMap we have this is in the persistence code
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069520056
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -315,6 +315,12 @@ private PolarisBaseEntity getPolarisBaseEntity(String query) {
return results.getFirst();
}
} catch (SQLException e) {
+ // This look-up is used for checking if the realm is bootstrap or not.
+ // If we have 1 DB per realm it might happen that the realm is not boostrap
+ // at all.
Review Comment:
How about if take this is in my MetaStoreManager refactor ? https://github.com/apache/polaris/issues/1462
I have assigned it to me
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064825227
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
Also, custom donwstream builds will probably want to do the mapping in custom code and apply whatever logic fits their use cases.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #1482:
URL: https://github.com/apache/polaris/pull/1482#issuecomment-2845387054
This is a nice idea !
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069318919
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1\".db-kind=pgsql
Review Comment:
having these obviously test-like entries in the production config is not nice. Could you make another attempt at removing them? Maybe you meant to put them under the `test` or `it` Quarkus profiles?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069670438
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
We could preconfigure (`quarkus.datasource."realm1_ds".db-kind=pgsql`) a few (say 5) Data Sources and allow users to provide connection strings and map them to realms in runtime... but that still looks awkward to me.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #1482:
URL: https://github.com/apache/polaris/pull/1482#issuecomment-2844756603
> IIUC you mean removing default-datasource ? i think we need default being explicit called out from the configuration. I am not entirely sure what do you imply by `SmallRye's convention` ? can you please elaborate
@singhpk234 in SmallRye when you are mapping a `Map` you have the possibility to define a special key called the "unnamed" key. That key in the map will hold the default value for the map. You generally don't need to define an extra property to hold the default value, the default value will be read from the "root":
```java
@ConfigMapping(prefix = "polaris.relation.jdbc.datasource")
public interface RelationalJdbcConfiguration {
String DEFAULT_REALM_KEY = "<default>";
/** realmId to configured Datasource name mapping. */
@WithParentName
@WithUnnamedKey(DEFAULT_REALM_KEY)
Map<String, String> realms();
}
```
An example configuration would be:
```properties
# Default config:
polaris.relation.jdbc.datasource=realm1_ds
# Alternative configs per realm:
polaris.relation.jdbc.datasource.realm2=realm2_ds
polaris.relation.jdbc.datasource.realm3=realm3_ds
```
A typical usage would be:
```java
String dataSourceName;
if (relationalJdbcConfiguration.realms().containsKey(realmId)) {
dataSourceName = relationalJdbcConfiguration.realms().get(realmId);
} else {
dataSourceName =
relationalJdbcConfiguration.realms().get(RelationalJdbcConfiguration.DEFAULT_REALM_KEY);
}
```
This convention is used in many other places, so from a user XP perspective it would be nice to have the same here.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064799251
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
I see so essentially having this mapping without an explicit config to support 3 use case :
for ex consider realm1, realm2, realm3
1. realm1, realm2, realm3 are 1 DB each
2. realm1, realm2, realm3 are 1 DB and each table has 3 realms (as PK)
3. realm 1 can be DB realm2 and realm3 can be just 1 DB and each table as 2 PK
where as having a config which decides the isolation level just allows :
case 1, 2
Thoughts ? i think having config like that reduces un-intentional error
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064372190
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
What I mean is that the list of realms is restricted by `RealmContextConfiguration` right now.
Since we have a fixed list of realm IDs, we can have a fixed map realm ID -> DS name, e.g.
```
quarkus.datasource.dsA.jdbc.url=...
quarkus.datasource.dsB.jdbc.url=...
```
then
```
polaris.realm-contex.data-source.realm1=dsA
polaris.realm-contex.data-source.realm2=dsA
polaris.realm-contex.data-source.realm3=dsB
```
It's up to the Polaris Admin to provide this mapping. The default would be to map all realms to the "default" DS.
In java:
```
public interface RealmContextConfiguration {
Map<String, String> dataSource();
[...]
}
No other config is necessary.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069313898
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.relational.jdbc;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Map;
+
+@ConfigMapping(prefix = "polaris.relation.jdbc")
+public interface RelationalJdbcConfiguration {
Review Comment:
I'm not sure about `RelationalJdbcConfiguration` in `polaris-core` this type is specific to the JDBC persistence, which is an extension. I suppose `quarkus/server` can get this type from the JDBC module. IIRC it depends on JDBC already.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069531130
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
`quarkus.datasource.\"realm1_ds\".db-kind=pgsql` has to be defined at build time, that would be a problem for user of the binary distribution, I guess.
I'm surprised this is required, because the default datasource does not have to be defined at build time, if I'm not mistaken.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069555773
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
I looks like current approach is then a no-go for binary distribution users :thinking:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067578696
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +83,11 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public DatasourceSupplier datasourceSupplier(
+ @Any RelationalJdbcConfiguration relationalJdbcConfiguration,
+ @All List<InstanceHandle<DataSource>> datasources) {
+ return new QuarkusDatasourceSupplier(relationalJdbcConfiguration, datasources);
Review Comment:
Did you have `@ApplicationScoped`?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069670438
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
We could preconfigure (`quarkus.datasource."pg1".db-kind=pgsql`, etc) a few (say 5) Data Sources and allow users to provide connection strings and map them to realms in runtime... but that still looks awkward to me.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064053666
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1\".db-kind=pgsql
Review Comment:
This is for this behaviour
**_I observed some unexplainable quarkus behaviour and spent days on debugging it, but now i have some way to work around, with custom DS of quarkus if i don't specify them in application prop and even though i specify it my PostgresLifeCycleManagement test container it doesn't create these beans, so declearing even the garbage in the application prop does the trick._**
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067505748
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
sounds fair, i think being explicit would be really helpful, so i am also +1
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067688330
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -315,6 +315,12 @@ private PolarisBaseEntity getPolarisBaseEntity(String query) {
return results.getFirst();
}
} catch (SQLException e) {
+ // This look-up is used for checking if the realm is bootstrap or not.
+ // If we have 1 DB per realm it might happen that the realm is not boostrap
+ // at all.
Review Comment:
yes entity null would mean entity not found and this would prompt bootstrap which inturn would run the bootstrap script. Otherwise RTE here is everything broken, i agree this is not an ideal but this is something we have from persistence we expect EntityResult
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067743561
##########
quarkus/commons/src/main/java/org/apache/polaris/commons/QuarkusDatasourceSupplier.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.commons;
+
+import io.quarkus.arc.InstanceHandle;
+import java.util.List;
+import javax.sql.DataSource;
+import org.apache.polaris.extension.persistence.relational.jdbc.DatasourceSupplier;
+import org.apache.polaris.extension.persistence.relational.jdbc.RelationalJdbcConfiguration;
+
+public class QuarkusDatasourceSupplier implements DatasourceSupplier {
Review Comment:
moved this code to `quarkus/common` and moved the tests to `quarkus/test-commons'
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069514333
##########
gradle/projects.main.properties:
##########
@@ -30,6 +30,7 @@ polaris-quarkus-service=quarkus/service
polaris-quarkus-server=quarkus/server
polaris-quarkus-spark-tests=quarkus/spark-tests
polaris-quarkus-admin=quarkus/admin
+polaris-quarkus-commons=quarkus/commons
Review Comment:
I'm talking about naming: why commonS in the plural?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069561044
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
Multi datasource as implemented here will indeed require a rebuild of the application.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069536007
##########
polaris-core/src/main/java/org/apache/polaris/core/config/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.config;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Map;
+import java.util.Optional;
+
+@ConfigMapping(prefix = "polaris.relation.jdbc.datasource")
Review Comment:
Yes, good point about this config being internal to Quarkus modules.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #1482:
URL: https://github.com/apache/polaris/pull/1482#issuecomment-2843429552
@singhpk234 just to confirm, is the intent here:
1. To support a finite, pre-defined number of datasources _that are known at build time_ – or
2. To support an unbounded, undefined number of datasources _that are not known at build time_
?
Your current work hints at 1 since the datasources are currently managed by Quarkus, but the tests seem to expect 2, hence my confusion.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064062351
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
Are you suggesting if <realm-id> specific custom DS is there then use realm specific DS or otherwise use the default one if not there, would it not create a confusion if there are both ?
For example :
consider realm1, realm2 ....
and the admin configure
``` java
Map.entry("quarkus.datasource.realm1.db-kind", "pgsql"),
Map.entry("quarkus.datasource.realm1.active", "true"),
Map.entry("quarkus.datasource.realm1.jdbc.url", postgres.getJdbcUrl()),
Map.entry("quarkus.datasource.realm1.username", postgres.getUsername()),
Map.entry("quarkus.datasource.realm1.password", postgres.getPassword()),
Map.entry("quarkus.datasource.db-kind", "pgsql"),
Map.entry("quarkus.datasource.jdbc.url", postgres.getJdbcUrl()),
Map.entry("quarkus.datasource.username", postgres.getUsername()),
Map.entry("quarkus.datasource.password", postgres.getPassword()),
```
do we silently make the realm1 go to realm1 DS and realm2 to default DS ?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064822766
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
From my POV this is the Polaris Admin kind of decision. As long as Polaris provides sufficient mechanisms for admins to implement all supported strategies, it's up to the admins if they want to mix and match or isolate everything.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067577401
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -315,6 +315,12 @@ private PolarisBaseEntity getPolarisBaseEntity(String query) {
return results.getFirst();
}
} catch (SQLException e) {
+ // This look-up is used for checking if the realm is bootstrap or not.
+ // If we have 1 DB per realm it might happen that the realm is not boostrap
+ // at all.
Review Comment:
would returning `null` help with bootstrapping?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067538887
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -315,6 +315,12 @@ private PolarisBaseEntity getPolarisBaseEntity(String query) {
return results.getFirst();
}
} catch (SQLException e) {
+ // This look-up is used for checking if the realm is bootstrap or not.
+ // If we have 1 DB per realm it might happen that the realm is not boostrap
+ // at all.
Review Comment:
Its like we checked and table didn't exists, so this looks up should not fail but rather say that he nothing is bootstrappped and hence trigger actual bootstrap or purge or something
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069316361
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +83,11 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public DatasourceSupplier datasourceSupplier(
+ @Any RelationalJdbcConfiguration relationalJdbcConfiguration,
+ @All List<InstanceHandle<DataSource>> datasources) {
+ return new QuarkusDatasourceSupplier(relationalJdbcConfiguration, datasources);
Review Comment:
Was it indexed by Jandex?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069658474
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
That datasource is created manually and is not managed by Quarkus. We can do that of course, but my impression was that we want to keep using Quarkus-managed datasources.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069477210
##########
gradle/projects.main.properties:
##########
@@ -30,6 +30,7 @@ polaris-quarkus-service=quarkus/service
polaris-quarkus-server=quarkus/server
polaris-quarkus-spark-tests=quarkus/spark-tests
polaris-quarkus-admin=quarkus/admin
+polaris-quarkus-commons=quarkus/commons
Review Comment:
I'd suggest to merge the module `admin` and `server`. I didn't see much benefits, while it adds unnecessary burden for dev and release. I will share this idea in the dev mailing list later.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] [DO NOT REVIEW] Support multiple Quarkus datasources [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1482:
URL: https://github.com/apache/polaris/pull/1482#issuecomment-2951473454
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2078434342
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
@snazy we can't as we need atleast one build time prop https://github.com/apache/polaris/pull/1482#discussion_r2069488866
I tried overriding them by quarkus-test-profiles but the override is not able to create named DS
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2078452843
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
@singhpk234 : This PR accumulated a lot of comments, some of them about non-trivial issues... Would you mind making a dev email with a summary and options for moving forward? I guess it would be clearer than continuing on GH. Once we have a consensus on how to proceed we can revamp this PR. WDYT?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069315432
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -315,6 +315,12 @@ private PolarisBaseEntity getPolarisBaseEntity(String query) {
return results.getFirst();
}
} catch (SQLException e) {
+ // This look-up is used for checking if the realm is bootstrap or not.
+ // If we have 1 DB per realm it might happen that the realm is not boostrap
+ // at all.
Review Comment:
this logic is too obscure IMHO. Would it be preferable to have a dedicated `isRealmBootstrapped(realmId)` method?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069470866
##########
gradle/projects.main.properties:
##########
@@ -30,6 +30,7 @@ polaris-quarkus-service=quarkus/service
polaris-quarkus-server=quarkus/server
polaris-quarkus-spark-tests=quarkus/spark-tests
polaris-quarkus-admin=quarkus/admin
+polaris-quarkus-commons=quarkus/commons
Review Comment:
we wanted to share logic between quarkus/admin and quarkus/service hence this module :)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069380735
##########
polaris-core/src/main/java/org/apache/polaris/core/config/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.config;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Map;
+import java.util.Optional;
+
+@ConfigMapping(prefix = "polaris.relation.jdbc.datasource")
Review Comment:
I would rather remove the annotation here and create a sub-interface in a Quarkus module with the annotation.
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java:
##########
@@ -258,6 +265,13 @@ public ActiveRolesProvider activeRolesProvider(
return activeRolesProviders.select(Identifier.Literal.of(persistenceType)).get();
}
+ @Produces
+ public DatasourceSupplier datasourceSupplier(
+ @Any RelationalJdbcConfiguration relationalJdbcConfiguration,
+ @All List<InstanceHandle<DataSource>> datasources) {
+ return new QuarkusDatasourceSupplier(relationalJdbcConfiguration, datasources);
Review Comment:
This is just doing a regular bean construction injection, so it's likely possible to get rid of this and inject into `QuarkusDatasourceSupplier` directly.
##########
polaris-core/src/main/java/org/apache/polaris/core/config/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.config;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Map;
+import java.util.Optional;
+
+@ConfigMapping(prefix = "polaris.relation.jdbc.datasource")
Review Comment:
Hmm in fact, this interface is only used in Quarkus modules, so I'd suggest moving it to `quarkus-commons`.
##########
quarkus/commons/build.gradle.kts:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+plugins {
+ alias(libs.plugins.jandex)
Review Comment:
I feel we are missing
```
alias(libs.plugins.quarkus)
id("polaris-quarkus")
```
But indeed adding those doesn't work. Let's investigate later.
##########
quarkus/commons/build.gradle.kts:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+plugins {
+ alias(libs.plugins.jandex)
+ id("java-test-fixtures")
+}
+
+configurations.all {
+ exclude(group = "org.antlr", module = "antlr4-runtime")
+ exclude(group = "org.scala-lang", module = "scala-library")
+ exclude(group = "org.scala-lang", module = "scala-reflect")
+}
+
+java {
+ sourceCompatibility = JavaVersion.VERSION_21
+ targetCompatibility = JavaVersion.VERSION_21
+}
+
+dependencies {
+ implementation(project(":polaris-core"))
+ implementation(enforcedPlatform(libs.quarkus.bom))
+ implementation(enforcedPlatform(libs.quarkus.bom))
Review Comment:
```suggestion
implementation(enforcedPlatform(libs.quarkus.bom))
```
##########
gradle/projects.main.properties:
##########
@@ -30,6 +30,7 @@ polaris-quarkus-service=quarkus/service
polaris-quarkus-server=quarkus/server
polaris-quarkus-spark-tests=quarkus/spark-tests
polaris-quarkus-admin=quarkus/admin
+polaris-quarkus-commons=quarkus/commons
Review Comment:
nit: we already have `service/common` ("common" in the singular), any reason to have `quarkus/commons` ("commons" in the plural)?
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +83,11 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public DatasourceSupplier datasourceSupplier(
+ @Any RelationalJdbcConfiguration relationalJdbcConfiguration,
+ @All List<InstanceHandle<DataSource>> datasources) {
+ return new QuarkusDatasourceSupplier(relationalJdbcConfiguration, datasources);
Review Comment:
You are missing `@All` I guess:
```java
@Inject
public QuarkusDatasourceSupplier(
RelationalJdbcConfiguration relationalJdbcConfiguration,
@All List<InstanceHandle<DataSource>> dataSources) {
```
##########
quarkus/commons/build.gradle.kts:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+plugins {
+ alias(libs.plugins.jandex)
+ id("java-test-fixtures")
+}
+
+configurations.all {
Review Comment:
This is really surprising, but indeed we need this. There is some serious Gradle snafu going on.
##########
polaris-core/src/main/java/org/apache/polaris/core/config/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.config;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Map;
+import java.util.Optional;
+
+@ConfigMapping(prefix = "polaris.relation.jdbc.datasource")
+public interface RelationalJdbcConfiguration {
+ /** realmId to configured Datasource name mapping. */
+ Map<String, String> realm();
Review Comment:
```suggestion
Map<String, String> realms();
```
Also, you probably need to annotate this with `@WithParentName` and maybe `@WithUnnamedKey`, depending on how you want to handle the "default" Jdbc configuration.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064053666
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1\".db-kind=pgsql
Review Comment:
yes, to be precise, This is for this behaviour
**_I observed some unexplainable quarkus behaviour and spent days on debugging it, but now i have some way to work around, with custom DS of quarkus if i don't specify them in application prop and even though i specify it my PostgresLifeCycleManagement test container it doesn't create these beans, so declearing even the garbage in the application prop does the trick._**
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064866900
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
I think it's important that the configuration capture the notion of a **realm strategy** -- i.e. do I route requests to a metastore only based on an explicit configuration for that realm, or do I send all requests to some metastore by default?
Whether that's done explicitly by configuring a strategy or implicitly through the presence (or absence) of a default configuration, I think it's fine. Consider a setup like the following:
The admin has configured DSs for some individual realms:
```
polaris.realm-contex.data-source.realm.realm-1=dsA
polaris.realm-contex.data-source.realm.realm-2=dsA
polaris.realm-contex.data-source.realm.realm-3=dsB
```
A new realm `realm-4` shows up -- I expect things to not work as the system does not know where to route the request.
Later, the admin configures polaris with a new default:
```
polaris.realm-contex.data-source.default=dsC
```
Now, I expect new realms `realm-4` and `realm-5` to get routed to `dsC`. If the admin later "promotes" `realm-4` to have its own DS, I would expect the service to respect that.
The important thing is that the system does not route requests to some invisible default backend; the admin explicitly must configure the system to route unknown (unconfigured) realms to some specific DS.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064799251
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
I see so essentially having this mapping without an explicit config to support 3 use case :
for ex consider realm1, realm2, realm3
1. realm1, realm2, realm3 are 1 DB each
2. realm1, realm2, realm3 are 1 DB and each table has 3 realms (as PK)
3. realm 1 can be DB realm2 and realm3 can be just 1 DB and each table as 2 PK
where as having a config which decides the isolation level just allows :
case 1, 2
Thoughts ? i think having config like that reduces un-intentional error
test
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
I see so essentially having this mapping without an explicit config to support 3 use case :
for ex consider realm1, realm2, realm3
1. realm1, realm2, realm3 are 1 DB each
2. realm1, realm2, realm3 are 1 DB and each table has 3 realms (as PK)
3. realm 1 can be DB realm2 and realm3 can be just 1 DB and each table as 2 PK
where as having a config which decides the isolation level just allows :
case 1, 2
Thoughts ? i think having config like that reduces un-intentional error
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] [DO NOT REVIEW] Support multiple Quarkus datasources [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #1482: [JDBC] [DO NOT REVIEW] Support multiple Quarkus datasources
URL: https://github.com/apache/polaris/pull/1482
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067489063
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
not sure whats happening but here is the reason behind : https://github.com/apache/polaris/pull/1482#discussion_r2064041402
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069569429
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
I guess we could follow an approach similar to [this](https://github.com/projectnessie/nessie/blob/1fbaaa511a3fc9a62d8432b5b2d08a6606f2e478/gc/gc-repository-jdbc/src/main/java/org/projectnessie/gc/contents/jdbc/AgroalJdbcDataSourceProvider.java#L173), plus add some CDI lifecycle to shut those Data Sources down when the application shuts down.
@adutra : WDYT?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #1482:
URL: https://github.com/apache/polaris/pull/1482#issuecomment-2843503400
> polaris.relational.jdbc.datasource.realm-1=dsA
> polaris.relational.jdbc.datasource.realm-2=dsB
> polaris.relational.jdbc.datasource.default-datasource=dsC
I suggest though that you adopt SmallRye's convention for named things:
```properties
polaris.relational.jdbc.datasource=dsC # default (unnamed) datasource
polaris.relational.jdbc.datasource.realm-1=dsA # named datasource "realm-1"
polaris.relational.jdbc.datasource.realm-2=dsB # named datasource "realm-2"
```
You will find an example of that here (from #1397):
https://github.com/apache/polaris/blob/d51cb1afe3abf2329bf2e4ccf74c57c26f514661/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/QuarkusAuthenticationConfiguration.java#L36
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #1482:
URL: https://github.com/apache/polaris/pull/1482#issuecomment-2843461823
@adutra here is what i want to achieve : https://github.com/apache/polaris/pull/1482#discussion_r2064866900
polaris.relational.jdbc.datasource.realm-1=dsA
polaris.relational.jdbc.datasource.realm-2=dsB
polaris.relational.jdbc.datasource.default-datasource=dsC
for realm-1, realm-2 we go to dsA and dsB both need to predefined
realm-3 shows up since the mapping is not there it goes to default-datasource i.e dsC
essentially this would expecte all dsA, dsB, dsC configured otherwise we fail.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069477220
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +83,11 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public DatasourceSupplier datasourceSupplier(
+ @Any RelationalJdbcConfiguration relationalJdbcConfiguration,
+ @All List<InstanceHandle<DataSource>> datasources) {
+ return new QuarkusDatasourceSupplier(relationalJdbcConfiguration, datasources);
Review Comment:
works 👍
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064376123
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
If you prefer we could have a different config class for that `Map`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064033081
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcDatasource.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+
+public interface JdbcDatasource {
Review Comment:
nit: Maybe `DataSourceSupplier`?
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -304,6 +304,9 @@ private PolarisBaseEntity getPolarisBaseEntity(String query) {
return results.getFirst();
}
} catch (SQLException e) {
+ if (e.getSQLState().equals("42P01")) {
Review Comment:
This logic should probably be in `DatabaseType`
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusJdbcDataSource.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.admintool.config;
+
+import io.quarkus.arc.InstanceHandle;
+import java.util.List;
+import javax.sql.DataSource;
+import org.apache.polaris.extension.persistence.relational.jdbc.JdbcDatasource;
+import org.postgresql.shaded.com.ongres.scram.common.util.Preconditions;
+
+public class QuarkusJdbcDataSource implements JdbcDatasource {
+ private final List<InstanceHandle<DataSource>> dataSources;
+ private final String realmIsolationType;
+
+ public static final String DEFAULT_DATA_SOURCE_NAME = "<default>";
+ public static final String REALM_ISOLATION_TYPE_DB = "db";
+
+ public QuarkusJdbcDataSource(String realmIsolationType, List<InstanceHandle<DataSource>> dataSources) {
+ this.realmIsolationType = realmIsolationType;
+ this.dataSources = dataSources;
+ }
+
+ @Override
+ public DataSource fromRealmId(String realmId) {
+ Preconditions.checkNotNull(REALM_ISOLATION_TYPE_DB.equals(realmIsolationType) && dataSources.size() == 1, "Misconfigured datasources as per realm requirements");
+ for (InstanceHandle<DataSource> handle : dataSources) {
+ String name = handle.getBean().getName();
+ name = name == null ? DEFAULT_DATA_SOURCE_NAME : unquoteDataSourceName(name);
+ // if realm isolation is DB then there should be only one DS configured.
+ if (name.equals(realmId) || (REALM_ISOLATION_TYPE_DB.equals(realmIsolationType) && name.equals(DEFAULT_DATA_SOURCE_NAME))) {
+ return handle.get();
+ }
+ }
+ throw new IllegalStateException("No datasource configured with name: " + realmId);
+ }
+
+ public static String unquoteDataSourceName(String dataSourceName) {
+ if (dataSourceName.startsWith("\"") && dataSourceName.endsWith("\"")) {
Review Comment:
It might be best to define a config `Map` for translating realm ID to DS names (cf. `RealmContextConfiguration`), defaulting to 1:1 association when not configured.
##########
quarkus/test-commons/src/main/java/org/apache/polaris/test/commons/PostgresRelationalJdbcLifeCycleManagement.java:
##########
@@ -53,19 +56,40 @@ public Map<String, String> start() {
context.containerNetworkId().ifPresent(postgres::withNetworkMode);
postgres.start();
- return Map.of(
- "polaris.persistence.type",
- "relational-jdbc",
- "quarkus.datasource.db-kind",
- "pgsql",
- "quarkus.datasource.jdbc.url",
- postgres.getJdbcUrl(),
- "quarkus.datasource.username",
- postgres.getUsername(),
- "quarkus.datasource.password",
- postgres.getPassword(),
- "quarkus.datasource.jdbc.initial-size",
- "10");
+
+ if (separateDs) {
Review Comment:
is this really useful for this class, which manages the database container? Would it not be clearer for it to have the list of databases to pre-configure?
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1\".db-kind=pgsql
Review Comment:
Is this for testing?
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
What is the purpose of the `polaris.persistence.realm.isolation.type` config? Considering that there could be many Polaris servers, each running with different config, this settings is rather confusion IMHO.
A given Polaris server merely needs a way to find a DataSource for each Realm ID.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067665004
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +83,11 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public DatasourceSupplier datasourceSupplier(
+ @Any RelationalJdbcConfiguration relationalJdbcConfiguration,
+ @All List<InstanceHandle<DataSource>> datasources) {
+ return new QuarkusDatasourceSupplier(relationalJdbcConfiguration, datasources);
Review Comment:
yup
```
@ApplicationScoped
public class QuarkusDatasourceSupplier implements DatasourceSupplier {
private final List<InstanceHandle<DataSource>> dataSources;
private final RelationalJdbcConfiguration relationalJdbcConfiguration;
private static final String DEFAULT_DATA_SOURCE_NAME = "default";
@Inject
public QuarkusDatasourceSupplier(
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2078434342
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
@snazy we can't as we need atleast one build time prop please ref this comment from @adutra https://github.com/apache/polaris/pull/1482#discussion_r2069488866
I tried overriding them by quarkus-test-profiles but the override is not able to create named DS
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067521323
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +83,11 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public DatasourceSupplier datasourceSupplier(
+ @Any RelationalJdbcConfiguration relationalJdbcConfiguration,
+ @All List<InstanceHandle<DataSource>> datasources) {
+ return new QuarkusDatasourceSupplier(relationalJdbcConfiguration, datasources);
Review Comment:
I tried it but it didn't worked for me, here is what i did
- remove the code from producer to make this class object
- made the constructor of QuarkusDatasourceSupplier injectable
```
@Inject
public QuarkusDatasourceSupplier(
RelationalJdbcConfiguration relationalJdbcConfiguration,
List<InstanceHandle<DataSource>> dataSources) {
this.relationalJdbcConfiguration = relationalJdbcConfiguration;
this.dataSources = dataSources;
}
```
It failed complaining
```
Caused by: jakarta.enterprise.inject.UnsatisfiedResolutionException: Unsatisfied dependency for type org.apache.polaris.extension.persistence.relational.jdbc.DatasourceSupplier and qualifiers [@Default]
- injection target: org.apache.polaris.extension.persistence.relational.jdbc.JdbcMetaStoreManagerFactory#jdbcDatasource
- declared on CLASS bean [types=[org.apache.polaris.extension.persistence.relational.jdbc.JdbcMetaStoreManagerFactory, java.lang.Object, org.apache.polaris.core.persistence.MetaStoreManagerFactory], qualifiers=[@Identifier("relational-jdbc"), @Any], target=org.apache.polaris.extension.persistence.relational.jdbc.JdbcMetaStoreManagerFactory]
at io.quarkus.arc.processor.Beans.resolveInjectionPoint(Beans.java:547)
at io.quarkus.arc.processor.BeanInfo.init(BeanInfo.java:689)
at io.quarkus.arc.processor.BeanDeployment.init(BeanDeployment.java:323)
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064053666
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1\".db-kind=pgsql
Review Comment:
This is for this behaviour
```
I observed some unexplainable quarkus behaviour and spent days on debugging it, but now i have some way to work around, with custom DS of quarkus if i don't specify them in application prop and even though i specify it my PostgresLifeCycleManagement test container it doesn't create these beans, so declearing even the garbage in the application prop does the trick.
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064372190
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
What I mean is that the list of realms is restricted by `RealmContextConfiguration` right now.
Since we have a fixed list of realm IDs, we can have a fixed map realm ID -> DS name, e.g.
```
quarkus.datasource.dsA.jdbc.url=...
quarkus.datasource.dsB.jdbc.url=...
```
then
```
polaris.realm.realm1.ds=dsA
polaris.realm.realm2.ds=dsA
polaris.realm.realm3.ds=dsB
```
It's up to the Polaris Admin to provide this mapping. The default would be to map all realms to the "default" DS.
No other config is necessary.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Support 1 DS per realm [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2064866900
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,9 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public JdbcDatasource jdbcDatasource(@ConfigProperty(name = "polaris.persistence.realm.isolation.type") String realmIsolationType, @All List<InstanceHandle<DataSource>> dataSources) {
Review Comment:
I think it's important that the configuration capture the notion of a **realm strategy** -- i.e. do I route requests to a metastore only based on an explicit configuration for that realm, or do I send all requests to some metastore by default?
Whether that's done explicitly by configuring a strategy or implicitly through the presence (or absence) of a default configuration, I think it's fine. Consider a setup like the following:
The admin has configured DSs for some individual realms:
```
polaris.realm-contex.data-source.realm.realm-1=dsA
polaris.realm-contex.data-source.realm.realm-2=dsA
polaris.realm-contex.data-source.realm.realm-3=dsB
```
A new realm `realm-4` shows up -- I expect things to not work as the system does not know where to route the request. Later, the admin configures polaris with a new default:
```
polaris.realm-contex.data-source.default=dsC
```
Now, I expect new realms `realm-4` and `realm-5` to get routed to `dsC`. If the admin later "promotes" `realm-4` to have its own DS, I would expect the service to respect that.
The important thing is that the system does not route requests to some invisible default backend; the admin explicitly must configure the system to route unknown (unconfigured) realms to some specific DS.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069542232
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.relational.jdbc;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Map;
+
+@ConfigMapping(prefix = "polaris.relation.jdbc")
+public interface RelationalJdbcConfiguration {
Review Comment:
Agreed, I liked Alex suggestion, moved this to quarkus/commons https://github.com/apache/polaris/pull/1482#discussion_r2069388646
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.relational.jdbc;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Map;
+
+@ConfigMapping(prefix = "polaris.relation.jdbc")
+public interface RelationalJdbcConfiguration {
Review Comment:
Agreed, I liked Alex suggestion as well, moved this to quarkus/commons https://github.com/apache/polaris/pull/1482#discussion_r2069388646
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069531130
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
If `quarkus.datasource.\"realm1_ds\".db-kind=pgsql` has to be defined at build time, that will be a problem for users of the binary distribution, I guess.
I'm surprised this is required, because the default datasource does not have to be defined at build time, if I'm not mistaken.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #1482:
URL: https://github.com/apache/polaris/pull/1482#issuecomment-2843620479
IIUC you mean removing default-datasource ? i think we need default being explicit called out from the configuration. I am not entirely sure what do you imply by `SmallRye's convention` ? can you please elaborate
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2069668972
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
It would be ideal to let Quarkus manage the Data Sources, but having to enumerate them at build time kills the user-level benefits, IMHO.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2065412396
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusDatasourceSupplier.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.admintool.config;
+
+import io.quarkus.arc.InstanceHandle;
+import java.util.List;
+import javax.sql.DataSource;
+import org.apache.polaris.extension.persistence.relational.jdbc.DatasourceSupplier;
+import org.apache.polaris.extension.persistence.relational.jdbc.RelationalJdbcConfiguration;
+
+public class QuarkusDatasourceSupplier implements DatasourceSupplier {
Review Comment:
I know this is a copy of the code in the service, but getting this in the commons causes a depedency hell I am working on checking how can i move this to common.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067462025
##########
quarkus/commons/src/main/java/org/apache/polaris/commons/PostgresRelationalJdbcLifeCycleManagement.java:
##########
@@ -53,19 +60,42 @@ public Map<String, String> start() {
context.containerNetworkId().ifPresent(postgres::withNetworkMode);
postgres.start();
- return Map.of(
- "polaris.persistence.type",
- "relational-jdbc",
- "quarkus.datasource.db-kind",
- "pgsql",
- "quarkus.datasource.jdbc.url",
- postgres.getJdbcUrl(),
- "quarkus.datasource.username",
- postgres.getUsername(),
- "quarkus.datasource.password",
- postgres.getPassword(),
- "quarkus.datasource.jdbc.initial-size",
- "10");
+ Map<String, String> props = new HashMap<>();
+ props.put("polaris.persistence.type", "relational-jdbc");
+
+ if (!databases.isEmpty()) {
+ for (String database : databases) {
+ // polaris.relation.jdbc.datasource.realm=realm_ds
+ props.put(String.format("polaris.relation.jdbc.datasource.%s", database), database + "_ds");
+ props.put(String.format("quarkus.datasource.%s.db-kind", database + "_ds"), "pgsql");
+ props.put(String.format("quarkus.datasource.%s.active", database + "_ds"), "true");
+ props.put(
+ String.format("quarkus.datasource.%s.jdbc.url", database + "_ds"),
+ postgres.getJdbcUrl().replace("realm1", database));
+ props.put(
+ String.format("quarkus.datasource.%s.username", database + "_ds"),
+ postgres.getUsername());
+ props.put(
+ String.format("quarkus.datasource.%s.password", database + "_ds"),
+ postgres.getPassword());
+ }
+ } else {
+ return Map.of(
+ "polaris.persistence.type",
+ "relational-jdbc",
+ "quarkus.datasource.db-kind",
+ "pgsql",
+ "quarkus.datasource.jdbc.url",
+ postgres.getJdbcUrl(),
+ "quarkus.datasource.username",
+ postgres.getUsername(),
+ "quarkus.datasource.password",
+ postgres.getPassword(),
+ "quarkus.datasource.jdbc.initial-size",
+ "10");
+ }
+
+ return props;
Review Comment:
The code patterns of falling through to here from the main `if` branch, but not doing so from `else` look very odd to me from code maintenance POV.
##########
quarkus/commons/src/main/java/org/apache/polaris/commons/QuarkusDatasourceSupplier.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.commons;
+
+import io.quarkus.arc.InstanceHandle;
+import java.util.List;
+import javax.sql.DataSource;
+import org.apache.polaris.extension.persistence.relational.jdbc.DatasourceSupplier;
+import org.apache.polaris.extension.persistence.relational.jdbc.RelationalJdbcConfiguration;
+
+public class QuarkusDatasourceSupplier implements DatasourceSupplier {
Review Comment:
It looks like we mixing this production code with test code in the same module, same package... This is not nice :thinking:
WDYT about keeping this class under `quarkus/service` (and keeping test code as before)?
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +83,11 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public DatasourceSupplier datasourceSupplier(
+ @Any RelationalJdbcConfiguration relationalJdbcConfiguration,
+ @All List<InstanceHandle<DataSource>> datasources) {
+ return new QuarkusDatasourceSupplier(relationalJdbcConfiguration, datasources);
Review Comment:
If you add an injectable constructor (same params) to `QuarkusDatasourceSupplier` then producers will not be required.
##########
quarkus/commons/src/main/java/org/apache/polaris/commons/QuarkusDatasourceSupplier.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.commons;
+
+import io.quarkus.arc.InstanceHandle;
+import java.util.List;
+import javax.sql.DataSource;
+import org.apache.polaris.extension.persistence.relational.jdbc.DatasourceSupplier;
+import org.apache.polaris.extension.persistence.relational.jdbc.RelationalJdbcConfiguration;
+
+public class QuarkusDatasourceSupplier implements DatasourceSupplier {
+ private List<InstanceHandle<DataSource>> dataSources;
+ private RelationalJdbcConfiguration relationalJdbcConfiguration;
Review Comment:
pls make these fields `final`
##########
quarkus/admin/src/main/resources/application.properties:
##########
@@ -49,3 +49,21 @@ quarkus.index-dependency.guava.group-id=com.google.guava
quarkus.index-dependency.guava.artifact-id=guava
quarkus.index-dependency.protobuf.group-id=com.google.protobuf
quarkus.index-dependency.protobuf.artifact-id=protobuf-java
+
+#quarkus.datasource.db-kind=pgsql
+#quarkus.datasource.jdbc.url=polaris
+#quarkus.datasource.username=polaris
+#quarkus.datasource.password=polaris
+quarkus.datasource.\"realm1_ds\".db-kind=pgsql
Review Comment:
I'm still not sure why we need the multitude of test-like datasource configs in the production sections of the admin tool properties :thinking:
##########
extension/persistence/relational-jdbc/build.gradle.kts:
##########
@@ -32,6 +32,7 @@ dependencies {
compileOnly(libs.jakarta.inject.api)
implementation(libs.smallrye.common.annotation) // @Identifier
+ compileOnly(libs.smallrye.config.core)
Review Comment:
Why do we need this?
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceSupplier.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.relational.jdbc;
+
+import javax.sql.DataSource;
+
+public interface DatasourceSupplier {
Review Comment:
It might be preferable to move this class to `polaris-core` to avoid service code depending on extensions.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.extension.persistence.relational.jdbc;
+
+import io.smallrye.config.ConfigMapping;
+import java.util.Map;
+
+@ConfigMapping(prefix = "polaris.relation.jdbc")
+public interface RelationalJdbcConfiguration {
Review Comment:
It is preferable to keep config interfaces used by more generic Polaris modules free from Quarkus dependencies, and them have a sub-interface (mostly empty) inside Quarkus modules with config annotations for build-time wiring.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -315,6 +315,12 @@ private PolarisBaseEntity getPolarisBaseEntity(String query) {
return results.getFirst();
}
} catch (SQLException e) {
+ // This look-up is used for checking if the realm is bootstrap or not.
+ // If we have 1 DB per realm it might happen that the realm is not boostrap
+ // at all.
Review Comment:
So why not throw an exception in that case?
##########
quarkus/commons/src/main/java/org/apache/polaris/commons/QuarkusDatasourceSupplier.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.commons;
+
+import io.quarkus.arc.InstanceHandle;
+import java.util.List;
+import javax.sql.DataSource;
+import org.apache.polaris.extension.persistence.relational.jdbc.DatasourceSupplier;
+import org.apache.polaris.extension.persistence.relational.jdbc.RelationalJdbcConfiguration;
+
+public class QuarkusDatasourceSupplier implements DatasourceSupplier {
+ private List<InstanceHandle<DataSource>> dataSources;
+ private RelationalJdbcConfiguration relationalJdbcConfiguration;
+
+ public static final String DEFAULT_DATA_SOURCE_NAME = "<default>";
Review Comment:
why `public`?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JDBC] Support multiple Quarkus datasources [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1482:
URL: https://github.com/apache/polaris/pull/1482#discussion_r2067487484
##########
quarkus/commons/src/main/java/org/apache/polaris/commons/QuarkusDatasourceSupplier.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.commons;
+
+import io.quarkus.arc.InstanceHandle;
+import java.util.List;
+import javax.sql.DataSource;
+import org.apache.polaris.extension.persistence.relational.jdbc.DatasourceSupplier;
+import org.apache.polaris.extension.persistence.relational.jdbc.RelationalJdbcConfiguration;
+
+public class QuarkusDatasourceSupplier implements DatasourceSupplier {
Review Comment:
I see may be i can put them under different directories, the thing i need this for both admin and service
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org