ctubbsii commented on a change in pull request #1873:
URL: https://github.com/apache/accumulo/pull/1873#discussion_r562914874
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java
##########
@@ -70,6 +81,20 @@ public static void setSystemProperty(ServerContext context,
String property, Str
public static void removeSystemProperty(ServerContext context, String
property)
throws InterruptedException, KeeperException {
+ AtomicBoolean shouldRemove = new AtomicBoolean(true);
+ DeprecatedPropertyUtil.getReplacementName(property, (log, replacement) -> {
+ log.warn("{} was deprecated and will be removed in a future release;"
+ + " no action was taken because it is not set here;"
+ + " did you mean to remove its replacment {} instead?", property,
replacement);
+ shouldRemove.set(false);
+ });
+ if (shouldRemove.get()) {
+ removePropWithoutDeprecationWarning(context, property);
+ }
Review comment:
@keith-turner The rationale is that when setting a deprecated property,
the intent is clear that the user is trying to change the behavior of Accumulo
by setting the property to some specific value. We can handle that intent by
translating to the new name when we store it. However, when a user is removing
a property, the intent is much less clear, and could be *very* unintuitive. It
would be very strange, for example, to ask to remove "X" (which does not
exist), and then to have "Y" deleted instead. Rather than rely on this
unintuitive behavior, I think it would be better to just do the safest thing
and make no changes, relying instead on the very verbose warning, so that the
user can be more explicit about what they intended to do.
Furthermore, removing a property already had the asymmetrical behavior of
not triggering any warning or error when removing a property that isn't a valid
zoo property, while setting the same property would have triggered an error.
The behavior in this PR is an extension of that existing NOOP behavior when an
invalid property is requested to be removed. This preserves the previous
behavior of the remove method, only adding a new warning.
Honestly, I preferred the solution where we just prevent the API from being
able to set the old `master.*` property names entirely. That way, it would just
get an error, and we wouldn't have to try to discern and accommodate user
intent. Yes, in the edge case where there might be some automation
setting/removing properties, that automation may fail as a result of no longer
being able to set/remove these properties, since they aren't recognized. But, I
think that would be far more preferable than trying to guess user intent and
try to communicate with users via warnings, introducing the possibility for
more mistakes (mistakes due to not expecting the automatic translation, or
mistakes due to expecting an automatic translation that doesn't occur). At
least with an error, the situation is clearly communicated to the caller.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
