kevinrr888 commented on code in PR #5632:
URL: https://github.com/apache/accumulo/pull/5632#discussion_r2192991232
##########
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 that is the case, then Property.getPropertyByKey(propertyString) will
probably return null and this will all blow up.
I don't think it will. If `Property.getPropertyByKey(property))` returns
null, `Property.isFixedZooPropertyKey(null)` === `return
FIXED_PROPERTIES.contains(null);` will return false, which is what we expect.
From documentation of EnumSet:
`
Null elements are not permitted. Attempts to insert a null element will
throw NullPointerException. Attempts to test for the presence of a null element
or to remove one will, however, function properly.
`
> 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.
The latter case is handled, so I don't think any follow on is needed for this
--
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]
