keith-turner commented on code in PR #5632:
URL: https://github.com/apache/accumulo/pull/5632#discussion_r2164851787
##########
core/src/main/java/org/apache/accumulo/core/util/compaction/CompactionServicesConfig.java:
##########
@@ -53,6 +53,7 @@ private long getDefaultThroughput(AccumuloConfiguration
aconf) {
return aconf.getAsBytes(Property.TSERV_MAJC_THROUGHPUT);
}
+ // TODO is this supposed to be getting the default value or should it be
reading from config?
Review Comment:
Can remove the TODO. This code will use a deprecated property if its set as
the default otherwise if falls back to the default value for the newer
property. This default is only used when per compaction service config does
not set a rate.
##########
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
Review Comment:
Is this only removing a useless paren? Seems like that is the case, want to
make sure I am not missing something.
##########
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))) {
+ String s = value == null ? String.format("Removing a fixed property %s.
", property)
+ : String.format("Setting a fixed property %s to value %s. ",
property, value);
+ log.warn(s + "Change will not take effect until related processes are
restarted.");
Review Comment:
Will this show up in the manager logs?
--
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]
