-
Notifications
You must be signed in to change notification settings - Fork 370
Feat: Added an option to fast forward the ledger state #1196
Feat: Added an option to fast forward the ledger state #1196
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.
Very good!
Liked your mock utils :-)
Only one change request...
src/test/java/com/iota/iri/service/milestone/impl/MilestoneServiceImplTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iota/iri/service/ledger/impl/LedgerServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iota/iri/service/milestone/impl/LatestSolidMilestoneTrackerImpl.java
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
|
||
@Before | ||
public void setUp() { | ||
SnapshotMockUtils.mockSnapshotProvider(snapshotProvider); |
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.
why not put this in the constructor?
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.
Because the mocked properties are NULL in the constructor.
public void setUp() { | ||
SnapshotMockUtils.mockSnapshotProvider(snapshotProvider); | ||
|
||
MilestoneViewModel.clear(); |
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.
shouldn't this be in After
?
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.
Other tests don't clean up the cached MilestoneViewModels so it can happen that we start this test with some cache Milestones, but you are right this should ALSO go into after the test.
public class SnapshotMockUtils { | ||
private static final int DEFAULT_MILESTONE_START_INDEX = 70000; | ||
|
||
private static final Hash DEFAULT_GENESIS_HASH = Hash.NULL_HASH; |
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.
why not use Hash.NULL_HASH
directly?
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.
To make it easier to reason about the logic and allow the "user" of this utils class to easily determine the important values for the mocked objects.
Otherwise anybody who uses this class would have to examine the code of the specific method to know which values are used for the mocked objects.
I would even consider making them public so tests can check against these values without reading them from the mocked objects (that might for example be usefull when testing the rollback functionality - atm I read them from the mocked object and assume that the value was not magically modified during any other call).
But this will be refined ans grow as I add more tests.
src/test/java/com/iota/iri/service/snapshot/impl/SnapshotMockUtils.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.
👌
Optional<MilestoneViewModel> milestone = milestoneService.findLatestProcessedSolidMilestoneInDatabase(); | ||
if (milestone.isPresent()) { | ||
snapshotService.replayMilestones(snapshotProvider.getLatestSnapshot(), milestone.get().index()); | ||
} |
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 nicer :-)
Description
This PR introduces a way to fast forward the ledger state after a restart of IRI. Previously we had to rescan all milestones to rebuild the ledger state. Especially for nodes that start off with a non-snapshotted database, this could take a really long time (a few hours). In combination with the upcoming refactor of the replayMilestones method this allows us to massively decrease the time and resources that are required for this operation (to a few seconds rather than hours).
Type of change
Checklist: