-
Notifications
You must be signed in to change notification settings - Fork 370
Feature: Adds progress bar and estimated time until the node is synced #1575
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice,
Will soon run it
MilestoneService milestoneService, LedgerService ledgerService, | ||
LatestMilestoneTracker latestMilestoneTracker, TransactionRequester transactionRequester) { | ||
MilestoneService milestoneService, LedgerService ledgerService, | ||
LatestMilestoneTracker latestMilestoneTracker, TransactionRequester transactionRequester) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes over the 120 columns limit
When I do auto-formatting on my IDE I get a different result 🤷♂️
Are you using the auto-formatting instructions from https://github.com/iotaledger/iri/blob/dev/STYLEGUIDE.md#formatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but anyways, I reverted all the accidental reformats.
@@ -165,7 +178,7 @@ public void trackLatestSolidMilestone() throws MilestoneException { | |||
logChange(currentSolidMilestoneIndex); | |||
|
|||
currentSolidMilestoneIndex = snapshotProvider.getLatestSnapshot().getIndex(); | |||
if(currentSolidMilestoneIndex == latestMilestoneTracker.getLatestMilestoneIndex()){ | |||
if (currentSolidMilestoneIndex == latestMilestoneTracker.getLatestMilestoneIndex()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -193,7 +206,8 @@ private void latestSolidMilestoneTrackerThread() { | |||
firstRun = false; | |||
|
|||
ledgerService.restoreLedgerState(); | |||
logChange(snapshotProvider.getInitialSnapshot().getIndex()); | |||
milestoneStartIndex = snapshotProvider.getInitialSnapshot().getIndex(); | |||
logChange(milestoneStartIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, while testing your code, my node refuses to get synced 🤷♂️
I tried to look again in the code more critically. Couldn't find the reason.. but found other things for you to fix :-P
/** | ||
* The actual start milestone index from which the node started from | ||
*/ | ||
private int milestoneStartIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you maybe remove this field due to two reasons:
-
Once it is here we get another object that is a source of
milestoneStartIndex
. Sometimes later another dev will come along and add a getter for this. Eventually the design will become more spaghetti. The way we try to prevent spaghetti design is by having all leaving "service" class stateless by depend on other stateless "service" classes, and having data come from data providers so that we have one source of truth. -
IIUC, you only initialize this field once when the tracker service first runs. So if the node comes out of sync again but doesn't get shut down (e.g. internet connection was lost), then the next time the starting point will be wrong... so I think this is a bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables are now in an inner class.
* Used to keep track of the times when the last N milestones got applied: | ||
* used to calculate the average time needed to apply a milestone. | ||
*/ | ||
private ArrayBlockingQueue<Long> lastMilestoneApplyTimes = new ArrayBlockingQueue<>(20); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also should be cleared after the node is synced...
What I think will be the best design for the new two fields is to create a private inner class SyncProgressBar
(or whatever other appropriate name) that will hold those two fields.
Every time the node gets synced you clear them both.
All the methods that manipulate this progress bar will be in this class.
Of course AsciiProgressBar
will remain a public utility class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am now hiding the variables in an inner class. States get reset on each application where the node is synced.
int progressBlocks = (int) (10 * percentage); | ||
StringBuilder progressBarSB = new StringBuilder(progressBlocks); | ||
progressBarSB.append("["); | ||
for (int i = 0; i < 10; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional:
Usually numbers are private static final
Since this is a small utility class I care less whether you change it or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you referring to?
Node finally starts syncing after a few restarts: I got my first log output to start from 17%. Then no log appears for a while, then it starts from 6%:
Not a big deal... if you can fix it quick great! If not then we can live with this. |
You're seeing 17% first because the node didn't know about the actual real latest milestone: |
2 other things I just noticed:
When the progress bar is at 99% there seem to be 3 empty blocks. I would expect 1 or 0 empty blocks. Also the seconds to get synced to fluctuate quite a lot... It looks really funny.
If you have other ideas they are also welcome :-) |
I truncated the progress bar, because 3 blocks are equal 10%, so now it simply is 10 blocks instead of 30s. Makes the progress bar also smaller. |
I've upped the amount of entries in the application times to 100 so get a better average. Milestone applications are very random in nature of how long they take because usually a batch of them gets applied at once or it can be stagnant for a longer period of time, depending on how the node actually can solidify a given milestone. Just adding a static value imho would not make it better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more style request which will be enforced by checkstyle
/** | ||
* Holds variables containing information needed for sync progress calculation. | ||
*/ | ||
private static class SyncProgressInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As part of automating annoying requests I opened #1577 (please review) which forces to put inner classes in the end.
So just please cut and paste this to the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved
/** | ||
* Used to calculate the average time needed to apply a milestone. | ||
*/ | ||
private ArrayBlockingQueue<Long> lastMilestoneApplyTimes = new ArrayBlockingQueue<>(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you are cutting and pasting you can change the declarative type to BlockingQueue
if you want.
We have the LooseCoupling configured in PMD but apparently it doesn't catch this...
So technically it is something we enforce, but since it can't be automated (I believe annoying rules should be automated) and it is a private member in a private class then I won't be insistent on it. Just try to abstract what you can in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
BUG REPORT |
Description
Adds a progress bar and estimated time (in seconds) until the node is synced .
Example (log shortened, full log: https://ybin.me/p/cb75fc8bf6dd365b#K1ibi/csfBkxD/yv6sNLAeKuF8iGX9ms44C3iIhGfRs=):
Fixes # (issue)
#1572
Type of change
How Has This Been Tested?
Running it on a node.
Checklist: