keith-turner commented on code in PR #5861:
URL: https://github.com/apache/accumulo/pull/5861#discussion_r2323654040
##########
core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java:
##########
@@ -484,44 +485,66 @@ private ValidFateConfig(Set<Fate.FateOperation>
allFateOps, String name) {
@Override
public boolean test(String s) {
- final Set<Fate.FateOperation> seenFateOps;
+ final Set<Fate.FateOperation> seenFateOps = new HashSet<>();
+ final int maxPoolNameLen = 64;
try {
final var json = JsonParser.parseString(s).getAsJsonObject();
- seenFateOps = new HashSet<>();
for (var entry : json.entrySet()) {
- var key = entry.getKey();
- var val = entry.getValue().getAsInt();
- if (val <= 0) {
+ var poolName = entry.getKey();
+
+ if (poolName.length() > maxPoolNameLen) {
+ log.warn(
+ "Unexpected property value {} for {}. Configured name {} is
too long (> {} characters). Property was unchanged",
+ s, name, poolName, maxPoolNameLen);
+ return false;
+ }
+
+ var poolConfigSet = entry.getValue().getAsJsonObject().entrySet();
+ if (poolConfigSet.size() != 1) {
+ log.warn(
+ "Unexpected property value {} for {}. Expected one entry for
{} but saw {}. Property was unchanged",
+ s, name, poolName, poolConfigSet.size());
+ return false;
+ }
+
+ var poolConfig = poolConfigSet.iterator().next();
+
+ var poolSize = poolConfig.getValue().getAsInt();
+ if (poolSize <= 0) {
log.warn(
- "Invalid entry {} in {}. Must be a valid thread pool size.
Property was unchanged.",
- entry, name);
+ "Unexpected property value {} for {}. Must be a valid thread
pool size (>0), saw {}. Property was unchanged",
+ s, name, poolSize);
return false;
}
- var fateOpsStrArr = key.split(",");
+
+ var fateOpsStrArr = poolConfig.getKey().split(",");
for (String fateOpStr : fateOpsStrArr) {
Fate.FateOperation fateOp = Fate.FateOperation.valueOf(fateOpStr);
Review Comment:
This is an existing problem, if fateOpStr is not a valid enum it will throw
an exception.
```suggestion
try{
fateOp = Fate.FateOperation.valueOf(fateOpStr);
} catch(IllegalArg e) {
log.warn(...)
return false;
}
```
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -457,30 +457,30 @@ public enum Property {
PropertyType.TIMEDURATION, "Limit calls from metric sinks to zookeeper
to update interval.",
"1.9.3"),
MANAGER_FATE_USER_CONFIG("manager.fate.user.config",
-
"{\"TABLE_CREATE,TABLE_DELETE,TABLE_RENAME,TABLE_ONLINE,TABLE_OFFLINE,NAMESPACE_CREATE,"
+
"{'pool1':{'TABLE_CREATE,TABLE_DELETE,TABLE_RENAME,TABLE_ONLINE,TABLE_OFFLINE,NAMESPACE_CREATE,"
Review Comment:
Instead of pool[123], could use the names `general`, `commit`, `split`.
##########
core/src/main/java/org/apache/accumulo/core/fate/Fate.java:
##########
@@ -182,8 +183,10 @@ public void run() {
while (fateExecutorsIter.hasNext()) {
var fateExecutor = fateExecutorsIter.next();
- // if this fate executors set of fate ops is no longer present in
the config...
- if (!poolConfigs.containsKey(fateExecutor.getFateOps())) {
+ // if this fate executors set of fate ops is no longer present in
the config OR
+ // this fate executor was renamed in the config
+ if (!poolConfigs.containsKey(fateExecutor.getFateOps()) ||
!poolConfigs
+
.get(fateExecutor.getFateOps()).getKey().equals(fateExecutor.getName())) {
Review Comment:
I agree, keeping the code simpler for this case is best. Is there a test
that covers moving a fate operation between pools. Like changing the config
such that `TABLE_CANCEL_COMPACT` moves from `pool1` to `pool2`?
##########
core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java:
##########
@@ -484,44 +485,66 @@ private ValidFateConfig(Set<Fate.FateOperation>
allFateOps, String name) {
@Override
public boolean test(String s) {
- final Set<Fate.FateOperation> seenFateOps;
+ final Set<Fate.FateOperation> seenFateOps = new HashSet<>();
+ final int maxPoolNameLen = 64;
try {
final var json = JsonParser.parseString(s).getAsJsonObject();
- seenFateOps = new HashSet<>();
for (var entry : json.entrySet()) {
- var key = entry.getKey();
- var val = entry.getValue().getAsInt();
- if (val <= 0) {
+ var poolName = entry.getKey();
+
+ if (poolName.length() > maxPoolNameLen) {
+ log.warn(
+ "Unexpected property value {} for {}. Configured name {} is
too long (> {} characters). Property was unchanged",
+ s, name, poolName, maxPoolNameLen);
+ return false;
+ }
+
+ var poolConfigSet = entry.getValue().getAsJsonObject().entrySet();
+ if (poolConfigSet.size() != 1) {
+ log.warn(
+ "Unexpected property value {} for {}. Expected one entry for
{} but saw {}. Property was unchanged",
+ s, name, poolName, poolConfigSet.size());
+ return false;
+ }
+
+ var poolConfig = poolConfigSet.iterator().next();
+
+ var poolSize = poolConfig.getValue().getAsInt();
Review Comment:
Looking at the javadoc, there are a few exceptions this can throw if the
value is not an integer. Could improve the checking here and test non integer
values.
```java
try {
poolSize = poolConfig.getValue().getAsInt();
}catch (RuntimeException e){
log.warn(...)
return false;
}
```
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
