-
Notifications
You must be signed in to change notification settings - Fork 370
MilestoneTracker and LedgerValidator rework #1151
MilestoneTracker and LedgerValidator rework #1151
Conversation
…into merge_milestonetracker_rework
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.
Great job!!!
Code is tops :-)
I mainly have javadocs nits that are not inline with the styleguide.
src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iota/iri/service/milestone/LatestMilestoneTracker.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iota/iri/service/milestone/SeenMilestonesRetriever.java
Outdated
Show resolved
Hide resolved
if (currentSolidMilestoneIndex < latestMilestoneTracker.getLatestMilestoneIndex()) { | ||
MilestoneViewModel nextMilestone; | ||
while (!Thread.currentThread().isInterrupted() && | ||
(nextMilestone = MilestoneViewModel.get(tangle, currentSolidMilestoneIndex + 1)) != null && |
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 think that this should be bound by latestMilestoneTracker.getLatestMilestoneIndex()
because you are doing an extra read from the db
Seems like a minor issue
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.
Since the latestMilestoneTracker currently scans all candidates at the start it can take a really long time to have the correct value - If we find another solid milestone here I think it should be processed without waiting for the latestMilestoneTracker to advance to this milestone (thats also the reason why we have the method that syncs with the latestMilestoneTracker further down in the code).
src/main/java/com/iota/iri/service/milestone/SeenMilestonesRetriever.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iota/iri/service/milestone/impl/LatestSolidMilestoneTrackerImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iota/iri/service/milestone/impl/LatestSolidMilestoneTrackerImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iota/iri/service/milestone/impl/LatestSolidMilestoneTrackerImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iota/iri/service/milestone/impl/LatestSolidMilestoneTrackerImpl.java
Outdated
Show resolved
Hide resolved
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.
Great Job
Description
To prepare and simplify the next PR that enables the remaining features, we introduce a new milestone and ledger package that contains a rework of the named classes. They conform to the new code guidelines (using interfaces and fully documented).
Type of change
How Has This Been Tested?
Additional Unit Tests will be written later.
Checklist:
Please delete items that are not relevant.