ctubbsii commented on a change in pull request #1873:
URL: https://github.com/apache/accumulo/pull/1873#discussion_r563484228
##########
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:
> As a user I would prefer the experience where it continues to work in
2.1 (with warnings) and blows up at runtime in 2.2, 3.0, etc.
"it continues to work" is something I would prefer also, but it's not that
simple, because there isn't a single behavior that is expected. There are
multiple distinct behaviors, and the phrase "it continues to work" is ambiguous
about which ones should be preserved and which ones are acceptable to be
changed.
Here are some of the behaviors of 2.0 that people might expect:
A. Configurability: `{ } -> set X -> { X }`
B. Independence: `{ X } -> set Y -> { X, Y } -> remove X -> { Y }`
C. Reversibility: `{ } -> set X -> { X } -> remove X -> { }`
D. NOOP on removing missing: `{ Y } -> remove X -> { Y }`
With the automatic translation of of `X -> Y`, no matter what, some of these
expectations are going to be broken with any of the proposed behaviors for 2.1:
1. `{ } -> set X -> { Y } (with WARN)` - breaks independence (the intent of
this PR, so this break is acceptable when limited to the properties being
upgraded)
2. `{ Y } -> remove X -> { } (with WARN)` - your proposal; preserves
reversibility, but breaks NOOP on removing missing
3. `{ Y } -> remove X -> { Y } (with WARN)` - my proposal; preserves NOOP on
removing missing, but preserves reversibility
4. `{ Y } -> remove X -> ERROR` - breaks NOOP on removing missing
5. `{ Y } -> set X -> ERROR` - breaks configurability of the old properties,
but at least could be consistent with option 4 if used in conjunction with that
> Errors are not my personal preference, but its consistent at least and not
confusing.
If we accept that there are different user expectations and we can't
reliably infer user intent, then at least we can be unambiguous and perfectly
clear. However, I'm not certain if it makes more sense to be clear with only
ERROR messages, or also with thrown exceptions, because adding new thrown
exceptions on the remove case is breaking existing behavior (NOOP on removing
missing). For the set case, it wouldn't be that unusual, since we already have
some properties which cannot be set in ZooKeeper, which generate an exception.
These same properties do not generate a corresponding exception when requested
to be removed from ZooKeeper, so there's already an asymmetry there.
##########
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:
> As a user I would prefer the experience where it continues to work in
2.1 (with warnings) and blows up at runtime in 2.2, 3.0, etc.
"it continues to work" is something I would prefer also, but it's not that
simple, because there isn't a single behavior that is expected. There are
multiple distinct behaviors, and the phrase "it continues to work" is ambiguous
about which ones should be preserved and which ones are acceptable to be
changed.
Here are some of the behaviors of 2.0 that people might expect:
A. Configurability: `{ } -> set X -> { X }`
B. Independence: `{ X } -> set Y -> { X, Y } -> remove X -> { Y }`
C. Reversibility: `{ } -> set X -> { X } -> remove X -> { }`
D. NOOP on removing missing: `{ Y } -> remove X -> { Y }`
With the automatic translation of of `X -> Y`, no matter what, some of these
expectations are going to be broken with any of the proposed behaviors for 2.1:
1. `{ } -> set X -> { Y } (with WARN)` - breaks independence (the intent of
this PR, so this break is acceptable when limited to the properties being
upgraded)
2. `{ Y } -> remove X -> { } (with WARN)` - your proposal; preserves
reversibility, but breaks NOOP on removing missing
3. `{ Y } -> remove X -> { Y } (with WARN)` - my proposal; preserves NOOP on
removing missing, but breaks reversibility
4. `{ Y } -> remove X -> ERROR` - breaks NOOP on removing missing
5. `{ Y } -> set X -> ERROR` - breaks configurability of the old properties,
but at least could be consistent with option 4 if used in conjunction with that
> Errors are not my personal preference, but its consistent at least and not
confusing.
If we accept that there are different user expectations and we can't
reliably infer user intent, then at least we can be unambiguous and perfectly
clear. However, I'm not certain if it makes more sense to be clear with only
ERROR messages, or also with thrown exceptions, because adding new thrown
exceptions on the remove case is breaking existing behavior (NOOP on removing
missing). For the set case, it wouldn't be that unusual, since we already have
some properties which cannot be set in ZooKeeper, which generate an exception.
These same properties do not generate a corresponding exception when requested
to be removed from ZooKeeper, so there's already an asymmetry there.
##########
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:
> As a user I would prefer the experience where it continues to work in
2.1 (with warnings) and blows up at runtime in 2.2, 3.0, etc.
"it continues to work" is something I would prefer also, but it's not that
simple, because there isn't a single behavior that is expected. There are
multiple distinct behaviors, and the phrase "it continues to work" is ambiguous
about which ones should be preserved and which ones are acceptable to be
changed.
Here are some of the behaviors of 2.0 that people might expect:
A. Configurability: `{ } -> set X -> { X }`
B. Independence: `{ X } -> set Y -> { X, Y } -> remove X -> { Y }`
C. Reversibility: `{ } -> set X -> { X } -> remove X -> { }`
D. NOOP on removing missing: `{ Y } -> remove X -> { Y }`
With the automatic translation of of `X -> Y`, no matter what, some of these
expectations are going to be broken with any of the proposed behaviors for 2.1:
1. `{ } -> set X -> { Y } (with WARN)` - breaks independence (the intent of
this PR, so this break is acceptable when limited to the properties being
upgraded)
2. `{ Y } -> remove X -> { } (with WARN)` - your proposal; preserves
reversibility, but breaks NOOP on removing missing
3. `{ Y } -> remove X -> { Y } (with WARN)` - my proposal; preserves NOOP on
removing missing, but breaks reversibility
4. `{ Y } -> remove X -> ERROR` - breaks NOOP on removing missing
5. `{ } -> set X -> ERROR` - breaks configurability of the old properties,
but at least could be consistent with option 4 if used in conjunction with that
> Errors are not my personal preference, but its consistent at least and not
confusing.
If we accept that there are different user expectations and we can't
reliably infer user intent, then at least we can be unambiguous and perfectly
clear. However, I'm not certain if it makes more sense to be clear with only
ERROR messages, or also with thrown exceptions, because adding new thrown
exceptions on the remove case is breaking existing behavior (NOOP on removing
missing). For the set case, it wouldn't be that unusual, since we already have
some properties which cannot be set in ZooKeeper, which generate an exception.
These same properties do not generate a corresponding exception when requested
to be removed from ZooKeeper, so there's already an asymmetry there.
##########
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:
> As a user I would prefer the experience where it continues to work in
2.1 (with warnings) and blows up at runtime in 2.2, 3.0, etc.
"it continues to work" is something I would prefer also, but it's not that
simple, because there isn't a single behavior that is expected. There are
multiple distinct behaviors, and the phrase "it continues to work" is ambiguous
about which ones should be preserved and which ones are acceptable to be
changed.
Here are some of the behaviors of 2.0 that people might expect:
A. Configurability: `{ } -> set X -> { X }`
B. Independence: `{ X } -> set Y -> { X, Y } -> remove X -> { Y }`
C. Reversibility: `{ } -> set X -> { X } -> remove X -> { }`
D. NOOP on removing missing: `{ Y } -> remove X -> { Y }`
With the automatic translation of of `X -> Y`, no matter what, some of these
expectations are going to be broken with any of the proposed behaviors for 2.1:
1. `{ } -> set X -> { Y } (with WARN)` - breaks independence (the intent of
this PR, so this break is acceptable when limited to the properties being
upgraded)
2. `{ Y } -> remove X -> { } (with WARN)` - your proposal; preserves
reversibility, but breaks NOOP on removing missing
3. `{ Y } -> remove X -> { Y } (with WARN)` - my proposal; preserves NOOP on
removing missing, but breaks reversibility
4. `{ Y } -> remove X -> ERROR` - breaks NOOP on removing missing
5. `{ } -> set X -> ERROR` - breaks configurability of the old properties,
but at least could be consistent with option 4 if used in conjunction with that
> Errors are not my personal preference, but its consistent at least and not
confusing.
If we accept that there are different user expectations and we can't
reliably infer user intent, then at least we can be unambiguous and perfectly
clear. However, I'm not certain if it makes more sense to be clear with only
ERROR messages, or also with thrown exceptions, because adding new thrown
exceptions on the remove case is breaking existing behavior (NOOP on removing
missing). For the set case, it wouldn't be that unusual, since we already have
some properties which cannot be set in ZooKeeper, which generate an exception.
These same properties do not generate a corresponding exception when requested
to be removed from ZooKeeper, so there's already an asymmetry there.
> If master aliases are only supported for set and not delete ...
I don't think it has ever been a matter of *if* they are supported, but one
of *how* (as in, which specific behaviors are preserved, and which are broken).
##########
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:
Okay. That's fine. I'm not entirely convinced, but I've made my case,
and I am in the minority. I can submit another update to Brian's code to apply
the behavior advocated for by Keith and Brian. I appreciate both of your
patience in letting me advocate for my perspective, but I'm content to let it
go at this point, and proceed with the majority viewpoint.
----------------------------------------------------------------
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]
