ctubbsii commented on code in PR #3167:
URL: https://github.com/apache/accumulo/pull/3167#discussion_r1088130572
##########
core/src/main/java/org/apache/accumulo/core/fate/Fate.java:
##########
@@ -235,6 +235,9 @@ public Fate(T environment, TStore<T> store,
Function<Repo<T>,String> toLogStrFun
* Launches the specified number of worker threads.
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
+ if (executor != null) {
+ throw new IllegalStateException("Fate.startTransactionRunners called
twice.");
+ }
Review Comment:
Could rely on memozing this, so it can't be created twice, and you can also
get the effect of lazily creating it when first used, so you don't need to call
a start method to get it running.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1012,17 +1014,29 @@ private SortedMap<TServerInstance,TabletServerStatus>
gatherTableInformation(
badServers.remove(server);
}
}
- });
- }
- tp.shutdown();
- try {
- tp.awaitTermination(Math.max(10000, rpcTimeout / 3),
TimeUnit.MILLISECONDS);
- } catch (InterruptedException e) {
- log.debug("Interrupted while fetching status");
+ }));
+ }
+ long timeToWaitForCompletion = Math.max(10000, rpcTimeout / 3);
+ long currTime = System.currentTimeMillis();
Review Comment:
Do not rely on `System.currentTimeMillis()` for tracking duration of
executions. That is wall clock time, and can be changed by NTP, arbitrarily by
a user, a time zone change, daylight savings, leap time, etc. For timing
duration of events inside Java, always use `System.nanoTime()`, not because you
actually get nanosecond granularity (you probably don't), but because
`nanoTime(t2) - nanoTime(t1)` has stronger guarantees.
--
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]
