dennishuo commented on code in PR #3396: URL: https://github.com/apache/polaris/pull/3396#discussion_r2696991926
########## runtime/defaults/src/main/resources/application.properties: ########## @@ -283,6 +297,8 @@ quarkus.arc.ignored-split-packages=\ ## Quarkus required setting for third party indexing # fixed at build-time +quarkus.index-dependency.agrona.group-id=org.agrona +quarkus.index-dependency.agrona.artifact-id=agrona Review Comment: If I'm understanding correctly, *indexing* isn't the new thing, but `agrona` is the new library we need to index now in addition to e.g. avro, and guava, is that right? I do agree a bit that agrona is less ubiquitous the others here, so could be worth a quick comment in here saying what agrona does (looks like it's a fancy utils library? https://github.com/aeron-io/agrona) In my experience avro, guava, and protobuf are ubiquitous enough that no one thinks twice seeing it in deps, but agrona might be new to some folks (like me, or maybe I'm just old-fashioned :) ) I like to lean towards over-communicating rather than risk under-communicating in comments in these cases, since things like compliance audits, etc., will have folks looking through these types of configs and anything that can avoid unnecessary questions will help save time for everyone ########## runtime/defaults/src/main/resources/application.properties: ########## @@ -128,16 +131,27 @@ polaris.features."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] # realm overrides # polaris.features.realm-overrides."my-realm"."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"=true -# polaris.persistence.type=in-memory-atomic +# Available types: +# - in-memory - InMemoryPolarisMetaStoreManagerFactory +# - in-memory-atomic - InMemoryAtomicOperationMetaStoreManagerFactory +# - nosql (beta) - NoSQL persistence backend, define the backend type via 'polaris.persistence.nosql.backend' +# - relational-jdbc polaris.persistence.type=in-memory -# polaris.persistence.type=relational-jdbc +# Database backend for 'nosql' persistence-type +# Available backends: +# - InMemory - for testing purposes +# - MongoDb - configure the via the Quarkus extension +polaris.persistence.nosql.backend=InMemory Review Comment: +1 to commenting out ########## runtime/defaults/src/main/resources/application.properties: ########## @@ -41,6 +41,9 @@ quarkus.micrometer.enabled=true quarkus.micrometer.export.prometheus.enabled=true quarkus.oidc.enabled=true quarkus.otel.enabled=true +quarkus.mongodb.metrics.enabled=true Review Comment: +1 to commenting out everything that's specific to enabling the particular persistence type. Someone reading through the config file trying to figure out could get confused whether a mongodb config setting is active or impacts runtime behaviors despite not turning on mongodb for anything. Is there a way for the code-level default to already have these things enabled, and have a commented-out config basically reflect the code default (for these and the other similar mongodb configs discussed elsewhere). I'm thinking of how things like `hadoop-env.sh` or `spark-env.sh` or `spark-defaults.conf` are basically just a big file of commented-out settings that mostly just reflect code-level defaults, but make it easy for a reader to mix in what they want to customize without getting overwhelmed about bare-minimum settings. Are Quarkus application.properties "composeable" to easily define e.g. a `mongodb-mixins.properties` or `spanner-mixins.properties` to still make it easy to choose a persistence profile without ending up with a monolithic base config file? If not, big commented-out sections still seems a reasonable way to scale this. ########## runtime/defaults/src/main/resources/application.properties: ########## @@ -128,16 +131,27 @@ polaris.features."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] # realm overrides # polaris.features.realm-overrides."my-realm"."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"=true -# polaris.persistence.type=in-memory-atomic +# Available types: +# - in-memory - InMemoryPolarisMetaStoreManagerFactory +# - in-memory-atomic - InMemoryAtomicOperationMetaStoreManagerFactory +# - nosql (beta) - NoSQL persistence backend, define the backend type via 'polaris.persistence.nosql.backend' +# - relational-jdbc polaris.persistence.type=in-memory -# polaris.persistence.type=relational-jdbc +# Database backend for 'nosql' persistence-type +# Available backends: +# - InMemory - for testing purposes +# - MongoDb - configure the via the Quarkus extension +polaris.persistence.nosql.backend=InMemory polaris.secrets-manager.type=in-memory # if set to true it will try to start localstack at build and run time for the local environment # https://docs.quarkiverse.io/quarkus-amazon-services/dev/amazon-rds.html#_configuration_reference for more details quarkus.rds.devservices.enabled=false quarkus.rds.sync-client.type=apache +## MongoDB specific configuration +quarkus.mongodb.database=polaris Review Comment: This config setting could definitely use a comment explaining what it is and what the options are. At first glance it's not clear what the config does. Are the options, for example, "polaris" vs "glue" or "unity" ;) Is it just the database *name* in mongodb to use when mongodb is used as the persistence layer? I'd also prefer to comment out the configs like this one that aren't "active" by default (actually, I would've preferred to comment out the `quarkus.rds` ones as well and have a nice config section for all the `quarkus.rds.*` settings that makes it clear which config setting enables the block of rds configs too). In my experience, it's nice to pare down the actual active config strings in the application.properties file to the true bare minimum for correct default out-of-the-box. Even the configs that match code-level defaults would be present but commented out as documentation so it's easy to see what can be adjusted, but without having to worry about whether it's a crucial input when the deployment mode isn't applicable for the config. -- 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]
