ctubbsii commented on code in PR #5632:
URL: https://github.com/apache/accumulo/pull/5632#discussion_r2198861564
##########
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:
So, I agree that the case I'm talking about appears to be handled. I'm just
saying it should be done more explicitly, so the code is easier to read. Right
now, the behavior is a bit implicit, and makes a lot of assumptions. It's not
wrong, but it doesn't make those baked in assumptions clear upon inspection. To
make it more explicit:
1. The method signature should receive a `Property` type instead of `String`
2. The calling code should call `Property.getPropertyByKey(string)` if
needed and pass a `Property` type
3. The calling code can also do the null check or the method can explicitly
check for the null case:
```suggestion
if (property != null && Property.isFixedZooPropertyKey(property)) {
```
These changes would make the built-in assumptions and behavior more
explicit, and make the code more maintainable.
--
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]
