kevinrr888 commented on code in PR #5953:
URL: https://github.com/apache/accumulo/pull/5953#discussion_r2420570837
##########
core/src/main/java/org/apache/accumulo/core/fate/Fate.java:
##########
@@ -258,6 +263,18 @@ public Fate(T environment, FateStore<T> store, boolean
runDeadResCleaner,
Function<Repo<T>,String> toLogStrFunc, AccumuloConfiguration conf,
ScheduledThreadPoolExecutor genSchedExecutor) {
this.store = FateLogger.wrap(store, toLogStrFunc, false);
+ this.fateConfig =
+ new FateConfiguration<>(environment, runDeadResCleaner, conf,
genSchedExecutor);
+
+ }
+
+ public void start() {
+ log.info("Start {} FATE", store.type());
+ keepRunning.set(true);
Review Comment:
```suggestion
```
Already set to true at initialization
##########
core/src/main/java/org/apache/accumulo/core/fate/Fate.java:
##########
@@ -74,9 +74,10 @@ public class Fate<T> {
private static final Logger log = LoggerFactory.getLogger(Fate.class);
private final FateStore<T> store;
- private final ScheduledFuture<?> fatePoolsWatcherFuture;
+ private ScheduledFuture<?> fatePoolsWatcherFuture;
private final AtomicInteger needMoreThreadsWarnCount = new AtomicInteger(0);
- private final ExecutorService deadResCleanerExecutor;
+ private ExecutorService deadResCleanerExecutor;
+ private final FateConfiguration<T> fateConfig;
Review Comment:
```suggestion
private final FateConfiguration<T> fateThreadsConfig;
```
along with other suggestion
##########
core/src/main/java/org/apache/accumulo/core/fate/Fate.java:
##########
@@ -247,6 +248,10 @@ public void run() {
}
}
+ private record FateConfiguration<T>(T environment, boolean runDeadResCleaner,
Review Comment:
```suggestion
private record FateThreadsConfiguration<T>(T environment, boolean
runDeadResCleaner,
```
A bit more descriptive
##########
test/src/main/java/org/apache/accumulo/test/fate/FlakyFate.java:
##########
@@ -43,6 +43,7 @@ public FlakyFate(T environment, FateStore<T> store,
Function<Repo<T>,String> toL
fateExecutors.add(new FlakyFateExecutor<>(this, environment,
poolConfig.getKey(),
poolConfig.getValue().getValue(), poolConfig.getValue().getKey()));
}
+ start();
}
Review Comment:
same thing here
##########
test/src/main/java/org/apache/accumulo/test/fate/FastFate.java:
##########
@@ -40,6 +40,7 @@ public FastFate(T environment, FateStore<T> store, boolean
runDeadResCleaner,
Function<Repo<T>,String> toLogStrFunc, AccumuloConfiguration conf) {
super(environment, store, runDeadResCleaner, toLogStrFunc, conf,
new ScheduledThreadPoolExecutor(2));
+ start();
}
Review Comment:
For consistency, should call start after the FastFate is created:
```
var fastFate = new FastFate(...);
fastFate.start();
```
Is a bit more confusing having some tests:
```
var fate = new Fate(...);
fate.start();
```
and others:
```
var fastFate = new FastFate(...);
```
##########
test/src/main/java/org/apache/accumulo/test/fate/SlowFateSplit.java:
##########
@@ -58,6 +58,7 @@ public SlowFateSplit(T environment, FateStore<T> store,
Function<Repo<T>,String>
fateExecutors.add(new SlowFateSplitExecutor(this, environment,
poolConfig.getKey(),
poolConfig.getValue().getValue(), poolConfig.getValue().getKey()));
}
+ start();
}
Review Comment:
same thing here
--
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]
