kevinrr888 commented on code in PR #5632:
URL: https://github.com/apache/accumulo/pull/5632#discussion_r2193097084
##########
server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java:
##########
@@ -95,21 +96,36 @@ private static String validateSystemProperty(ServerContext
context, SystemPropKe
// Find the property taking prefix into account
Property foundProp = null;
for (Property prop : Property.values()) {
- if (prop.getType() == PropertyType.PREFIX &&
property.startsWith(prop.getKey())
+ if ((prop.getType() == PropertyType.PREFIX &&
property.startsWith(prop.getKey()))
|| prop.getKey().equals(property)) {
foundProp = prop;
break;
}
}
- if ((foundProp == null || (foundProp.getType() != PropertyType.PREFIX
- && !foundProp.getType().isValidFormat(value)))) {
+ if (foundProp == null || (foundProp.getType() != PropertyType.PREFIX
+ && !foundProp.getType().isValidFormat(value))) {
IllegalArgumentException iae = new IllegalArgumentException(
"Ignoring property " + property + " it is either null or in an
invalid format");
log.trace("Attempted to set zookeeper property. Value is either null or
invalid", iae);
throw iae;
}
+ logIfFixed(property, value);
+
return property;
}
+
+ /**
+ * Done as a last step before the property is finally changed (e.g., after
validation). If the
+ * property is fixed, logs a warning that the property change will not take
effect until related
+ * processes are restarted.
+ */
+ private static void logIfFixed(String property, String value) {
+ if (Property.isFixedZooPropertyKey(Property.getPropertyByKey(property))) {
Review Comment:
> If it actually needs to be a String, it presumably would be because the
property key could be a property that starts with a prefix, and is not exactly
one of the fixed property enums of Property type. If that is the case, then
Property.getPropertyByKey(propertyString) will probably return null and this
will all blow up.
Yes, it could be the case that the property starts with a prefix and is not
exactly one of the `Property` enum. But the current code handles this. If
`Property.getPropertyByKey(propertyString))` returns null (which would occur in
this case), `Property.isFixedZooPropertyKey(null)` === `return
FIXED_PROPERTIES.contains(null);` will return false, which is what we expect.
> So, either the method signature should be changed to... OR, we need to
handle the case when Property.getPropertyByKey(propertyString) does not find a
Property for the given key, which the current implementation does not handle.
I believe the latter case _is_ handled. If
`Property.getPropertyByKey(propertyString)` does not find a `Property` for the
given string, it cannot be in the `FIXED_PROPERTIES` set, so we expect
`Property.isFixedZooPropertyKey(Property.getPropertyByKey(property))` ===
`Property.isFixedZooPropertyKey(null)` to return false, which it does.
So, I don't think any changes are needed here and this thread can be
resolved, but please let me know if you think otherwise.
--
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]
