ctubbsii commented on code in PR #5632:
URL: https://github.com/apache/accumulo/pull/5632#discussion_r2183535577
##########
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:
My main concern there was that we're passing a String type instead of a
Property type. 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.
However, if we only pass property key strings that always exactly match to
Property, then it makes no sense to have this method receive them as Strings
(if, for example, we only have fixed properties that are exactly a Property
type key, and never a key that merely starts with a prefix type). It should
more strictly receive Property types.
So, either the method signature should be changed to:
```
private static void logIfFixed(Property property, String value) {
```
and we resolve the Property before this method is called, 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.
--
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]
