Display World State Sync Progress in Logs #1645
Conversation
- compute estimated world state completion - display estimated world state completion in the logs - `CompleteTaskStep` now have access to a `LongSupplier` to retireve the number of pending requests - use a `RunnableCounter` to trigger displaying every 1000 requests completed - only show a new log when the estimation changes - added test to check the estimation computation
|
I’d suggest reporting the raw downloaded vs pending nodes rather than calculating the percentage. Otherwise it will fairly quickly get to 99.9% or even 100% complete and stay there for a day or so. I lernt that the hard way - my first dashboard tried to report it as a percent. :) |
Ok no problem |
|
|
||
| public class CompleteTaskStep { | ||
| private static final Logger LOG = LogManager.getLogger(); | ||
| private static final int DISPLAY_PROGRESS_STEP = 1000; |
There was a problem hiding this comment.
This is logging too often. Needs to be at least 100,000 maybe even a million. We request up to 3,840 node at a time (10 concurrent requests for 384 nodes).
There was a problem hiding this comment.
A million is too rare - I'd go for 100,000.
| final String newEstimatedWorldStateCompletion = | ||
| doubleFormatter.format(computeWorldStateSyncProgress()); | ||
| if (!Objects.equals(newEstimatedWorldStateCompletion, lastEstimatedWorldStateCompletion)) { | ||
| LOG.info("Estimated World State completion: {}", newEstimatedWorldStateCompletion); |
There was a problem hiding this comment.
As mentioned the percent complete winds up being misleading. This should probably be something like:
Downloaded {} world state nodes. At least {} nodes remaining.. That conveniently avoids most of the complexity too. :)
There was a problem hiding this comment.
I suggest keeping the percent as an estimation at the end of the log because i think it was the initial request.
| @@ -48,9 +49,14 @@ public void inc() { | |||
| public void inc(final long amount) { | |||
| stepCounter.addAndGet((int) amount); | |||
| backedCounter.inc(amount); | |||
| total++; | |||
There was a problem hiding this comment.
This isn't thread safe so total will wind up being the wrong value.
Actually stepCounter isn't thread safe either. It should be a final AtomicLong then this method becomes something like:
backedCounter.inc(amount);
if (stepCounter.addAndGet(amount) % step == 0) {
task.run();
}
And then you can just get the total with stepCounter.
There was a problem hiding this comment.
Yes it is true, this is what i did for the first implementation of this class. But during the review someone said that if we don't need the total then no need to use a modulo just store the incremental counter and reset to zero when hit the step. But now we need the total so i will implement this approach.
There was a problem hiding this comment.
Right, it's not that we need the total - it's that it's not thread-safe to assign the new AtomicInteger (nor is it safe to downcast the long to an int when incrementing it). You could wind up with all kinds of weird exceptions coming out of it because the AtomicInteger could be exposed to other threads before its constructor has actually completed (compilers and CPUs are free to reorder instructions as there's no memory fences involved).
…display-world-state-sync-progress-logs
…display-world-state-sync-progress-logs
metrics/core/src/main/java/tech/pegasys/pantheon/metrics/RunnableCounter.java
Outdated
Show resolved
Hide resolved
AbdelStark
deleted the
feature/pie-1712-display-world-state-sync-progress-logs
branch
PR description
CompleteTaskStepnow have access to aLongSupplierto retireve the number of pending requestsRunnableCounterto trigger displaying every 1000 requests completedFixed Issue(s)