poojanilangekar commented on code in PR #1959:
URL: https://github.com/apache/polaris/pull/1959#discussion_r2198894638
##########
polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java:
##########
@@ -34,11 +35,12 @@ public class BehaviorChangeConfiguration<T> extends
PolarisConfiguration<T> {
protected BehaviorChangeConfiguration(
String key,
Review Comment:
The case with 0 keys is not possible. We can't define a PolarisConfig
without a key. That was always the case. Unless you'd prefer if we annotate the
field with `@NonNull` in the constructor.
> You can write a valid config that violates the "primary key constraint" by
duplicating an entry.
The `registerConfiguration` function will actually disallow duplicate
entries and throw an `IllegalArgumentException`. That check is a part of the
existing code, this PR is not change it. This PR goes further and checks that
any existing `key` or `legacyKey` does not overlap with a new configuration
that is being registered (again both `key` and `legacyKeys`).
I think we're looking at two entirely different aspects of a configuration.
1. Declaring the configuration in `FeatureConfiguration` or
`BehaviorChangeConfiguration`.
2. Assigning values to a configuration which is done in
application.properties / JVM arguments.
The two are orthogonal to each other. We should impose constraints on how
PolarisConfigs are declared. We can't however impose constraints on how the
user defines these configs. If the user does have multiple definitions of the
same config, then we apply the one with the highest priority and discard the
rest. If indeed we should disallow multiple definitions, then any config that
has a default value defined should essentially disallow any overwrites in
application.properties. AFAIK, Quarkus doesn't do that and there isn't a way
for the app to determine such a condition.
--
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]
