[Fix] LS taking too long #1843
[Fix] LS taking too long #1843
Conversation
|
|
||
| /** | ||
| * Implements the snapshot service. See interface for more information. | ||
| * @param tangle acts as a database interface. | ||
| * @param snapshotProvider gives us access to the relevant snapshots. | ||
| * @param config configuration with snapshot specific settings. | ||
| */ | ||
| public SnapshotServiceImpl(Tangle tangle, SnapshotProvider snapshotProvider, SnapshotConfig config) { | ||
| public SnapshotServiceImpl(Tangle tangle, SnapshotProvider snapshotProvider, IotaConfig config) { |
| log.info("Generating entrypoints for {}", targetIndex); | ||
|
|
||
| // Co back a but below the milestone. Limited to maxDepth or genisis | ||
| int startIndex = Math.max(snapshotProvider.getInitialSnapshot().getIndex(), targetIndex - config.getMaxDepth() |
There was a problem hiding this comment.
Because of historical reasons max depth is configurable...
which is bad in this case...
maybe hardcode a constant greater than 15?
There was a problem hiding this comment.
btw, this is the reason why you shouldn't have changed the interface
it was coded this way to make sure stuff like this won't happen
There was a problem hiding this comment.
This used to be limited to 100 hardcoded due to outer_shell_size, whatever that means. However that meant i had to wait for 100 milestones to test, and seemed just idiotic anyway.
What if you want snapshot depth 5 and synced delay 5 as well?? super small db for the win!
There was a problem hiding this comment.
I think that would be dangerous on mainnet...
I think we shouldn't allow for this
src/main/java/com/iota/iri/service/snapshot/impl/SnapshotServiceImpl.java
Show resolved
Hide resolved
| List<Hash> approvees = getMilestoneApprovees(milestoneIndex, milestone); | ||
| for (Hash approvee : approvees) { | ||
| if (Thread.interrupted()) { | ||
| return null; |
There was a problem hiding this comment.
usually when one resets the interrupted status (you are implicitly doing it here) , one throws an InterruptedException (and resets the interrupted status when you catch the error)... or at least one doesn't reset the interruption status..
The reason is that technically when you look at this method you are only aware of its scope and you are not aware of whether this thread will die later on. Imagine a case where the thread was interrupted but continues to work, and on the next check
You can argue that in this specific context it will work.. it may break if the code changes but this will probably not happen. Even though this may be correct, I ask you: why do things the wrong way to begin with?
| * @return The amount of ms we go back under target snapshot for generation of solid entrypoints | ||
| */ | ||
| private int getSepDepth() { | ||
| return config.isTestnet() ? config.getMaxDepth() : Math.min(MIN_LS_DEPTH_MAINNET, config.getMaxDepth()); |
There was a problem hiding this comment.
I would still have the minimum for testnet... just to be sure
| if (Thread.currentThread().isInterrupted()) { | ||
| Thread.currentThread().interrupt(); | ||
| } |
There was a problem hiding this comment.
What you are doing here is setting the interrupt flag on...
But it was ready on and you didn't clear it now with isInterrupted... and you are not stopping the operation for sure...
So now this is completely broken
What I asked you before was either:
- Do it properly with
InterruptedException - Leave it like it was before but use
isInterruptedinstead ofinterruptedto not clear the flag. This would still be not the proper way to handle interrupts but at least it was less in your face bad than the original
So choose one of the above.. I'd read the links I sent you again and go with 1 just because I think you should probably know how interrupts in java work
This reverts commit d6cdd03.
Description of change
Changes code to use a more lightweight entrypoint generation.
Code largely based on Hornet LS code, thanks <3
fixes #1797
base branch should be changed, code is currently based on 1.8.5 as requested
Type of change
How the change has been tested
Takes snapshot, runs unit tests.
Test:
https://github.com/kwek20/random-tests/blob/master/src/main/java/org/iota/test/tests/TestSnapshot.java
Todo:
Private net testsCustom stitch tests