-
Notifications
You must be signed in to change notification settings - Fork 495
[JDBC] [DO NOT REVIEW] Support multiple Quarkus datasources #1482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
9de9f51
879378e
5c62e85
f2fa9b5
14ceabd
80304c3
6432732
32bb4b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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") | ||
|
singhpk234 marked this conversation as resolved.
Outdated
|
||
| public interface RelationalJdbcConfiguration { | ||
| /** realmId to configured Datasource name mapping. */ | ||
| Map<String, String> realm(); | ||
|
singhpk234 marked this conversation as resolved.
Outdated
|
||
|
|
||
| /** | ||
| * Default datasource name to be used for a realmId when there is no mapping of realmId to | ||
| * Datasource name present. | ||
| */ | ||
| Optional<String> defaultDatasource(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.core.persistence; | ||
|
|
||
| import javax.sql.DataSource; | ||
|
|
||
| public interface DatasourceSupplier { | ||
| DataSource fromRealmId(String realmId); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,3 +50,21 @@ 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.devservices.image-name=postgres:17-alpine | ||
|
|
||
| #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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still not sure why we need the multitude of test-like datasource configs in the production sections of the admin tool properties 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure whats happening but here is the reason behind : #1482 (comment)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: And you cannot pass build-time properties through So Thus for tests you have to declare the datasources you intend to use here, along with their db-kind: quarkus.datasource.\"realm1_ds\".db-kind=pgsql
quarkus.datasource.\"realm2_ds\".db-kind=pgsql
quarkus.datasource.\"realm3_ds\".db-kind=pgsql
# etcOther runtime properties, like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm surprised this is required, because the default datasource does not have to be defined at build time, if I'm not mistaken.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
See https://quarkus.io/guides/datasource#configure-multiple-datasources:
This is why I asked this question, I think there is some misunderstanding going on: #1482 (comment)
The default datasource is treated slightly different: if But that doesn't help here, since the intent is to use many named datasources.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not configure those DS using a quarkus-test-profile?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @snazy we can't as we need atleast one build time prop please ref this comment from @adutra #1482 (comment) I tried overriding them by quarkus-test-profiles but the override is not able to create named DS
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds fair ! its on my list, should be sending out an email soon, meanwhile marking this PR as draft to not draw more attention, |
||
| quarkus.datasource.\"realm1_ds\".jdbc.url=polaris | ||
| quarkus.datasource.\"realm1_ds\".username=polaris | ||
| quarkus.datasource.\"realm1_ds\".password=polaris | ||
| quarkus.datasource.\"realm2_ds\".db-kind=pgsql | ||
| quarkus.datasource.\"realm2_ds\".jdbc.url=polaris | ||
| quarkus.datasource.\"realm2_ds\".username=polaris | ||
| quarkus.datasource.\"realm2_ds\".password=polaris | ||
| quarkus.datasource.\"realm3_ds\".db-kind=pgsql | ||
| quarkus.datasource.\"realm3_ds\".jdbc.url=polaris | ||
| quarkus.datasource.\"realm3_ds\".username=polaris | ||
| quarkus.datasource.\"realm3_ds\".password=polaris | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| /* | ||
| * 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. | ||
| */ | ||
|
|
||
| -- Create two more databases for testing. The first database, polaris_realm1, is created | ||
| -- during container initialization. See PostgresTestResourceLifecycleManager. | ||
|
|
||
| -- Note: the database names must follow the pattern polaris_{realm}. That's the pattern | ||
| -- specified by the persistence.xml file used in tests. | ||
|
|
||
| CREATE DATABASE realm2; | ||
| GRANT ALL PRIVILEGES ON DATABASE realm2 TO polaris; | ||
|
|
||
| CREATE DATABASE realm3; | ||
| GRANT ALL PRIVILEGES ON DATABASE realm3 TO polaris; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel we are missing But indeed adding those doesn't work. Let's investigate later. |
||
| id("java-test-fixtures") | ||
| } | ||
|
|
||
| configurations.all { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really surprising, but indeed we need this. There is some serious Gradle snafu going on. |
||
| 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)) | ||
|
singhpk234 marked this conversation as resolved.
Outdated
|
||
| implementation("io.quarkus:quarkus-arc") | ||
| implementation(project(":polaris-relational-jdbc")) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why not throw an exception in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would returning
nullhelp with bootstrapping?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic is too obscure IMHO. Would it be preferable to have a dedicated
isRealmBootstrapped(realmId)method?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about if take this is in my MetaStoreManager refactor ? #1462
I have assigned it to me