kevinrr888 commented on code in PR #5861:
URL: https://github.com/apache/accumulo/pull/5861#discussion_r2322475021
##########
core/src/main/java/org/apache/accumulo/core/fate/Fate.java:
##########
@@ -276,23 +281,26 @@ public Fate(T environment, FateStore<T> store, boolean
runDeadResCleaner,
/**
* Returns a map of the current pool configurations as set in the given
config. Each key is a set
- * of fate operations and each value is an integer for the number of threads
assigned to work
- * those fate operations.
+ * of fate operations and each value is a map entry with key = fate executor
name and value = pool
+ * size
*/
@VisibleForTesting
- public static Map<Set<FateOperation>,Integer>
getPoolConfigurations(AccumuloConfiguration conf,
- FateInstanceType type) {
- Map<Set<FateOperation>,Integer> poolConfigs = new HashMap<>();
+ public static Map<Set<FateOperation>,Map.Entry<String,Integer>>
+ getPoolConfigurations(AccumuloConfiguration conf, FateInstanceType type)
{
+ Map<Set<FateOperation>,Map.Entry<String,Integer>> poolConfigs = new
HashMap<>();
final var json =
JsonParser.parseString(conf.get(getFateConfigProp(type))).getAsJsonObject();
for (var entry : json.entrySet()) {
- var key = entry.getKey();
- var val = entry.getValue().getAsInt();
- var fateOpsStrArr = key.split(",");
+ String fateExecutorName = entry.getKey();
+ var poolConfig =
entry.getValue().getAsJsonObject().entrySet().iterator().next();
+ String fateOpsStr = poolConfig.getKey();
+ int poolSize = poolConfig.getValue().getAsInt();
+ String[] fateOpsStrArr = fateOpsStr.split(",");
Set<FateOperation> fateOpsSet =
Arrays.stream(fateOpsStrArr).map(FateOperation::valueOf)
.collect(Collectors.toCollection(TreeSet::new));
- poolConfigs.put(fateOpsSet, val);
+ poolConfigs.put(fateOpsSet,
+ new AbstractMap.SimpleImmutableEntry<>(fateExecutorName, poolSize));
}
Review Comment:
no need to do any validation here, this is all handled before the prop is
set (in `PropertyType`)
##########
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:
would have been nice to avoid shutdown for just a pool rename, but would
make the code more complicated without shutdown: would need to unregister the
FateExecutorMetrics and register new ones, would need to rename running thread
pools, maybe other considerations as well, all of which are inherently handled
by shutdown
##########
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;
Review Comment:
not sure what a good max length is. Hard to imagine a useful name that would
be longer than 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]
