brianloss commented on a change in pull request #1873:
URL: https://github.com/apache/accumulo/pull/1873#discussion_r563693486
##########
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:
I'm in the camp of removing the replacement property if the deprecated
property name is sent to remove. I've been looking at this change though we're
telling users: "Property X has been replaced by property Y, but in order to be
less disruptive we are going to continue (for a short time) to allow you to use
X and behind the scenes we'll replace it with Y. If you use X, it's as though
you had used Y instead."
Given that we will have documented that using X is a substitute for using Y,
I'm struggling to come up with a good example where it is confusing or
unexpected for the user if they use the deprecated property name with remove
and we remove the replacement property (I'd say it's confusing not to do it).
The only case I can come with is if the user chooses to immediately start using
the new property names and then thinks they need to clean up zookeeper
themselves and remove the old properties so they call remove on the old names.
That usage is inconsistent with the documentation, and they'd figure it out
quickly with the warning that we're removing the replacement property names
instead. Are there other ways it could be confusing/unexpected for the user if
we remove the replacement property when they attempt to remove the deprecated
property?
----------------------------------------------------------------
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]
