flyrain commented on code in PR #1371:
URL: https://github.com/apache/polaris/pull/1371#discussion_r2059373336
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/models/ModelEntity.java:
##########
@@ -152,8 +152,8 @@ public ModelEntity fromResultSet(ResultSet r) throws
SQLException {
.purgeTimestamp(r.getObject("purge_timestamp", Long.class))
.toPurgeTimestamp(r.getObject("to_purge_timestamp", Long.class))
.lastUpdateTimestamp(r.getObject("last_update_timestamp", Long.class))
- .properties(r.getObject("properties", String.class))
- .internalProperties(r.getObject("internal_properties", String.class))
+ .properties(r.getString("properties"))
+ .internalProperties(r.getString("internal_properties"))
Review Comment:
Q: this is probably exact the same as before. Do we need this change?
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -50,6 +48,21 @@ public DatasourceOperations(DataSource datasource) {
this.datasource = datasource;
}
+ @Nonnull
+ public DatabaseType detect() {
+ try (Connection conn = borrowConnection()) {
+ String productName =
conn.getMetaData().getDatabaseProductName().toLowerCase(Locale.ROOT);
+ return switch (productName) {
+ case "h2" -> DatabaseType.H2;
+ case "postgresql" -> DatabaseType.POSTGRES;
+ default ->
+ throw new IllegalStateException("Unsupported DatabaseType: '" +
productName + "'");
+ };
+ } catch (SQLException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
Do we need to wrap it as its caller did that already?
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -67,13 +68,15 @@ public class JdbcMetaStoreManagerFactory implements
MetaStoreManagerFactory {
final Map<String, StorageCredentialCache> storageCredentialCacheMap = new
HashMap<>();
final Map<String, EntityCache> entityCacheMap = new HashMap<>();
final Map<String, Supplier<BasePersistence>> sessionSupplierMap = new
HashMap<>();
+ final List<DataSource> dataSources;
Review Comment:
Make it private?
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java:
##########
@@ -261,4 +266,13 @@ public ActiveRolesProvider activeRolesProvider(
public void closeTaskExecutor(@Disposes @Identifier("task-executor")
ManagedExecutor executor) {
executor.close();
}
+
+ @Produces
+ public List<DataSource> dataSources(@All List<InstanceHandle<DataSource>>
dataSources) {
+ List<DataSource> dataSourceList = new ArrayList<>();
+ for (InstanceHandle<DataSource> handle : dataSources) {
+ dataSourceList.add(handle.get());
+ }
+ return dataSourceList;
Review Comment:
With the fixed data source configurations, the [bootstrap tool
](https://polaris.apache.org/in-dev/unreleased/admin-tool/#bootstrapping-realms-and-principal-credentials)
won't work with JDBC, as it requires the data source to be pre-configured.
With that, adding a new account level tenant will require to stop the Polaris
server, to add new data source and to restart the server. This makes
database-base multi-tenancy impossible. We need to figure out a way to
dynamically add data sources. Can we file an issue and track that effort?
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -50,6 +48,21 @@ public DatasourceOperations(DataSource datasource) {
this.datasource = datasource;
}
+ @Nonnull
+ public DatabaseType detect() {
+ try (Connection conn = borrowConnection()) {
+ String productName =
conn.getMetaData().getDatabaseProductName().toLowerCase(Locale.ROOT);
+ return switch (productName) {
+ case "h2" -> DatabaseType.H2;
+ case "postgresql" -> DatabaseType.POSTGRES;
+ default ->
Review Comment:
Can we move this logic to be part of enum?
```
public static DatabaseType fromDisplayName(String displayName) {
return switch (displayName) {
case "h2" -> DatabaseType.H2;
case "postgresql" -> DatabaseType.POSTGRES;
default ->
throw new IllegalStateException("Unsupported DatabaseType: '"
+ displayName + "'");
};
}
```
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -106,12 +109,16 @@ private void initializeForRealm(
}
private DatasourceOperations getDatasourceOperations(boolean isBootstrap) {
- DatasourceOperations databaseOperations = new
DatasourceOperations(dataSource);
+ // TODO: remove when multiple dataSources are supported.
+ if (dataSources.size() > 1) {
+ throw new IllegalStateException("More than one dataSources configured");
+ }
+ DatasourceOperations databaseOperations = new
DatasourceOperations(dataSources.getFirst());
if (isBootstrap) {
- // TODO: see if we need to take script from Quarkus or can we just
- // use the script committed in the repo.
try {
-
databaseOperations.executeScript("scripts/postgres/schema-v1-postgres.sql");
+ DatabaseType databaseType = databaseOperations.detect();
Review Comment:
I feel it's not necessary to route this via the object of
`DatasourceOperations` as we've got the data source already. We could do
something like this:
```
var productName = dataSources.getFirst().getConnection()....
```
##########
quarkus/server/build.gradle.kts:
##########
@@ -50,6 +50,12 @@ dependencies {
runtimeOnly(project(":polaris-eclipselink"))
runtimeOnly("org.postgresql:postgresql")
+ runtimeOnly(project(":polaris-relational-jdbc"))
+ runtimeOnly("io.quarkus:quarkus-jdbc-postgresql") {
+ exclude(group = "org.antlr", module = "antlr4-runtime")
+ exclude(group = "org.scala-lang", module = "scala-library")
+ exclude(group = "org.scala-lang", module = "scala-reflect")
+ }
Review Comment:
Do we need this, as the module `polaris-quarkus-service` has it already?
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -67,13 +68,15 @@ public class JdbcMetaStoreManagerFactory implements
MetaStoreManagerFactory {
final Map<String, StorageCredentialCache> storageCredentialCacheMap = new
HashMap<>();
final Map<String, EntityCache> entityCacheMap = new HashMap<>();
final Map<String, Supplier<BasePersistence>> sessionSupplierMap = new
HashMap<>();
+ final List<DataSource> dataSources;
+
protected final PolarisDiagnostics diagServices = new
PolarisDefaultDiagServiceImpl();
- // TODO: Pending discussion of if we should have one Database per realm or 1
schema per realm
- // or realm should be a primary key on all the tables.
- @Inject DataSource dataSource;
@Inject PolarisStorageIntegrationProvider storageIntegrationProvider;
- protected JdbcMetaStoreManagerFactory() {}
+ @Inject
+ protected JdbcMetaStoreManagerFactory(List<DataSource> dataSources) {
Review Comment:
A `protected` constructor for CDI can work, but is a bit weird. Can we make
it a public constructor?
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -158,17 +167,25 @@ public void writeEntities(
entity.getParentId(),
entity.getTypeCode(),
entity.getName());
- if (exists != null) {
+ if (exists != null && !isUpdate) {
throw new EntityAlreadyExistsException(entity);
}
String query;
- if (originalEntities == null || originalEntities.get(i) == null)
{
+ if (!isUpdate) {
Review Comment:
Can we abstract two methods like this?
```
if (update) {
update()
} else {
create()
}
private update() {
}
private create() {
}
```
And this part could also be part of `create()` method. Another doubt is that
do we need to check existence before create a new entity? I think the
ConstraintViolation will happen anyways if we don't check.
```
// lookup by name
EntityNameLookupRecord exists =
lookupEntityIdAndSubTypeByName(
callCtx,
entity.getCatalogId(),
entity.getParentId(),
entity.getTypeCode(),
entity.getName());
if (exists != null && !isUpdate) {
throw new EntityAlreadyExistsException(entity);
}
```
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -93,8 +93,16 @@ public void writeEntity(
query = generateInsertQuery(modelEntity, realmId);
datasourceOperations.executeUpdate(query);
} catch (SQLException e) {
- if ((datasourceOperations.isConstraintViolation(e)
- || datasourceOperations.isAlreadyExistsException(e))) {
+ if (datasourceOperations.isConstraintViolation(e)) {
+ PolarisBaseEntity existingEntity =
+ lookupEntityByName(
+ callCtx,
+ entity.getCatalogId(),
+ entity.getParentId(),
+ entity.getTypeCode(),
+ entity.getName());
+ throw new EntityAlreadyExistsException(existingEntity, e);
+ } else if (datasourceOperations.isAlreadyExistsException(e)) {
Review Comment:
42P07 means a duplicated
table(https://www.postgresql.org/docs/8.2/errcodes-appendix.html), it doesn't
seem fit here, as we are not creating a new table here.
##########
extension/persistence/relational-jdbc/src/main/resources/postgres/schema-v1.sql:
##########
@@ -15,16 +15,12 @@
-- KIND, either express or implied. See the License for the
-- specific language governing permissions and limitations
-- under the License.
---
--- Note: Database and schema creation is not included in this script. Please
create the database and
--- schema before running this script. for example in psql:
--- CREATE DATABASE polaris_db;
--- \c polaris_db
--- CREATE SCHEMA polaris_schema;
--- set search_path to polaris_schema;
+CREATE SCHEMA IF NOT EXISTS POLARIS_SCHEMA;
+SET search_path TO POLARIS_SCHEMA;
CREATE TABLE IF NOT EXISTS entities (
+ realm_id TEXT NOT NULL,
Review Comment:
> Does max field size matter in Postgres if actual values are small?
No, only if the realmId is really long. I'm OK to leave it to other places
to apply the length control.
> A bigger concern, IMHO, is case sensitivity. I believe we should ensure
that realm IDs are case sensitive in the database.
It's by default case-sensitive as a string value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
