moved fate thread creation out of the constructor #5953
Conversation
kevinrr888
left a comment
kevinrr888
left a comment
There was a problem hiding this comment.
Can you run
mvn clean verify -Dspotbugs.skip -Dtest=skip -Dit.test=org.apache.accumulo.test.fate.**
to verify that all fate tests still pass?
|
I've run the command and it says that all test pass |
kevinrr888
left a comment
kevinrr888
left a comment
There was a problem hiding this comment.
Looks good, just some minor changes
| 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(); | ||
| } |
There was a problem hiding this 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(...);
| public FlakyFate(T environment, FateStore<T> store, Function<Repo<T>,String> toLogStrFunc, | ||
| AccumuloConfiguration conf) { | ||
| super(environment, store, false, toLogStrFunc, conf, new ScheduledThreadPoolExecutor(2)); | ||
| for (var poolConfig : getPoolConfigurations(conf, getStore().type()).entrySet()) { | ||
| fateExecutors.add(new FlakyFateExecutor<>(this, environment, poolConfig.getKey(), | ||
| poolConfig.getValue().getValue(), poolConfig.getValue().getKey())); | ||
| } | ||
| start(); | ||
| } |
| public SlowFateSplit(T environment, FateStore<T> store, Function<Repo<T>,String> toLogStrFunc, | ||
| AccumuloConfiguration conf) { | ||
| super(environment, store, false, toLogStrFunc, conf, new ScheduledThreadPoolExecutor(2)); | ||
| for (var poolConfig : getPoolConfigurations(conf, getStore().type()).entrySet()) { | ||
| fateExecutors.add(new SlowFateSplitExecutor(this, environment, poolConfig.getKey(), | ||
| poolConfig.getValue().getValue(), poolConfig.getValue().getKey())); | ||
| } | ||
| start(); | ||
| } |
DomGarguilo
left a comment
DomGarguilo
left a comment
There was a problem hiding this comment.
I think a few new things have to be considered with these changes. Specifically what should the behavior be in all combinations of calling .start() and .shutdown().
For example, if we do the following:
var fate = new Fate<>(...);
fate.start();
fate.start();
I think this will spin up new sets of internal resources (watcher, cleaner) and replace the old ones without the old ones being shut down. It might be best to require that a Fate object be shut down before calling .start() on it if we even want to allow for reuse of these objects. I am not sure.
Another scenario we need to consider is the following:
var fate = new Fate<>(...);
fate.shutdown();
Will probably get some NPE on resources that are created in the .start method now.
Seems like we need a new AtomicBoolean started now to track the started state and handle these cases properly.
3 participants
closes issue #5828
Moved Fate threads outside of the constructor and into a new start() method. Created a new FateConfig class to handle instantiations of variables being pass into the constructor and updated Fate calls to handle the new start method.