eric-maynard commented on code in PR #1170:
URL: https://github.com/apache/polaris/pull/1170#discussion_r1994291308
##########
polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java:
##########
@@ -37,6 +37,7 @@ protected BehaviorChangeConfiguration(
super(key, description, defaultValue, catalogConfig);
}
+ @BehaviorChangeConfigurationSince("1.0.0")
public static final BehaviorChangeConfiguration<Boolean>
VALIDATE_VIEW_LOCATION_OVERLAP =
Review Comment:
One point of order first -- it would be `1.2.0` when this becomes out of
date.
I think this question is getting at the motivation behind #1124 so I will
address it from that angle but please let me know if this doesn't make sense or
if your question is not addressed.
Why do we use behavior change flags at all? The idea is that some behavior
changes are considered high-risk or controversial -- i.e. there's a concern
that the change will cause a regression in some edge case, or that it will
negatively impact some fraction of users. Maybe there's a bug in Polaris, but
we fear some users are relying on the incorrect behavior.
For example, you may recall that Polaris
[previously](https://github.com/apache/polaris/pull/211) only took `true` for
`X-Iceberg-Access-Delegation` instead of the correct `vended-credentials`. In
that case, we maintained support for `true` to keep backwards compatibility for
clients. But what if we hadn't done that, or we didn't want to keep that
backwards compatibility around in perpetuity? A flag might have helped.
This is not to say that the behavior of accepting `true` was correct, and
indeed often only one side of a behavior change is really correct -- that's why
we're changing the behavior. But it can still be helpful to have the option to
revert back to the old, possibly incorrect, behavior just in case.
In summary, behavior change flags are intended to safeguard behavior changes
by providing a break-glass option to revert back to the pre-change behavior.
These are [transient
toggles](https://martinfowler.com/articles/feature-toggles.html), or [temporary
flags](https://launchdarkly.com/docs/guides/infrastructure/deployment-strategies#remove-temporary-flags).
They shouldn't stick around forever, lest the code become an untestable nest
of if/else blocks. #1170 is attempting to add a check to enforce this per
@flyrain's suggestion.
--
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]
