New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: added config variables for local snapshots #981

Merged
merged 1 commit into from Sep 13, 2018

Conversation

Projects
None yet
5 participants
@hmoog
Copy link
Contributor

hmoog commented Sep 11, 2018

Description

This PR introduces the config variables required for local snapshots.

    String LOCAL_SNAPSHOTS_ENABLED = "Flag that determines if local snapshots are enabled.";
    String LOCAL_SNAPSHOTS_PRUNING_ENABLED = "Flag that determines if pruning of old data is enabled.";
    String LOCAL_SNAPSHOTS_PRUNING_DELAY = "Only prune data that precedes the local snapshot by n milestones.";
    String LOCAL_SNAPSHOTS_INTERVAL_SYNCED = "Take local snapshots every n milestones if the node is fully synced.";
    String LOCAL_SNAPSHOTS_INTERVAL_UNSYNCED = "Take local snapshots every n milestones if the node is syncing.";
    String LOCAL_SNAPSHOTS_DEPTH = "Number of milestones to keep.";
    String LOCAL_SNAPSHOTS_BASE_PATH = "Path to the snapshot files (without file extensions).";

@jakubcech jakubcech referenced this pull request Sep 11, 2018

Closed

Review snapshot logic for local snapshots #980

5 of 6 tasks complete

@hmoog hmoog requested a review from karimodm Sep 12, 2018

@karimodm
Copy link
Member

karimodm left a comment

Looks good! Only things I pointed out are minor.
Also: do we want to test some of these in ConfigTest.java ?

int LOCAL_SNAPSHOTS_INTERVAL_SYNCED = 10;
int LOCAL_SNAPSHOTS_INTERVAL_UNSYNCED = 5000;
String LOCAL_SNAPSHOTS_BASE_PATH = "mainnet";
int LOCAL_SNAPSHOTS_DEPTH = 500;

This comment has been minimized.

@karimodm

karimodm Sep 12, 2018

Member

Do we have any sort of "story" backing up these default values?

This comment has been minimized.

@hmoog

hmoog Sep 12, 2018

Contributor

LOCAL_SNAPSHOTS_INTERVAL_SYNCED = 10
Taking local snapshots regularly makes the processing faster since it doesnt have to examine so many new transactions so we want to do it relatively often but also not too often since it still consumes as few resources - 5 minutes have proven to be a good value while testing to keep the processing times short (below a few seconds) and still don't bother the node too many times with creating the snapshots.

LOCAL_SNAPSHOTS_INTERVAL_UNSYNCED = 5000
When we are syncing we usually process a lot of milestones very rapidly. To not bother the syncing process too much but still have a fast resume point when IRI crashes for example, we create a snapshot every 5000 milestones. In addition to reducing the amount of local snapshots taken and the corresponding load on the node, it also creates a situation where we have a bigger chunk of the old tangle data available most of the time because the transactions only get pruned when a snapshot was taken. This allows us to revert the state of the ledger further back in case we detect a milestone transaction that was skipped previously and continue the syncing without an unrecoverable error - repairing milestones will be included in a future PR.

int LOCAL_SNAPSHOTS_DEPTH = 500;
I reduced the snapshot depth back to 500 because it works 99.9% of the time (i only once faced a situation where data was referenced by the coo that was older than 500 milestones. Since we also have a pruning delay now (new feature) which keeps old transactions prior to the taken snapshot., this situation should not be a problem anymore. At the same time a lower snapshot depth allows it to sync new nodes extremely fast.

This comment has been minimized.

@karimodm

karimodm Sep 12, 2018

Member

Since we also have a pruning delay now (new feature) which keeps old transactions prior to the taken snapshot.

So effectively you will be able to reference data up to LOCAL_SNAPSHOTS_DEPTH + LOCAL_SNAPSHOTS_PRUNING_DELAY milestones back, assuming pruning is enabled.

This comment has been minimized.

@hmoog

hmoog Sep 12, 2018

Contributor

yep

int LOCAL_SNAPSHOTS_PRUNING_DELAY = 1000;
int LOCAL_SNAPSHOTS_INTERVAL_SYNCED = 10;
int LOCAL_SNAPSHOTS_INTERVAL_UNSYNCED = 5000;
String LOCAL_SNAPSHOTS_BASE_PATH = "mainnet";

This comment has been minimized.

@karimodm

karimodm Sep 12, 2018

Member

Do we actually need this configuration variable? This should be automatically be adjusted depending to the network you are using. This way we can also always rely on a standard location for local snapshot files.

This comment has been minimized.

@hmoog

hmoog Sep 12, 2018

Contributor

since the file is currently persisted in the filesystem i would want to give the user the option to have control over the location of the file.

It might for example be possible that people run IOTA nodes with the folder of the binary being mounted as read only to secure the iri.jar against manipulations. In this case you might want to give a path to another directory where you store the working data like the db or the snapshot files.

This comment has been minimized.

@karimodm

karimodm Sep 12, 2018

Member

We already allow users to specify a data directory for their IRI nodes, I believe the local snapshots should be just dropped into that same folder; without giving the user this option, which I don't find the use for.

This comment has been minimized.

@alon-e

alon-e Sep 12, 2018

Member

the DB/persistable folder structure should be revisited, but as a different issue IMO.

This comment has been minimized.

@hmoog

hmoog Sep 12, 2018

Contributor

Can we create issues for minor things like this so i can change it afterwards? because once we have it merged i can more easily work on changes since i don't have to do them in 10 different branches.

This comment has been minimized.

@jakubcech

jakubcech Sep 12, 2018

Contributor

Yes, please, create an issue for this.

String LOCAL_SNAPSHOTS_PRUNING_DELAY = "Only prune data that precedes the local snapshot by n milestones.";
String LOCAL_SNAPSHOTS_INTERVAL_SYNCED = "Take local snapshots every n milestones if the node is fully synced.";
String LOCAL_SNAPSHOTS_INTERVAL_UNSYNCED = "Take local snapshots every n milestones if the node is syncing.";
String LOCAL_SNAPSHOTS_DEPTH = "Number of milestones to keep.";

This comment has been minimized.

@karimodm

karimodm Sep 12, 2018

Member

Is this the number of milestones to keep prior to latest local snapshot? Can you elaborate the description a bit?

This comment has been minimized.

@hmoog

hmoog Sep 12, 2018

Contributor

Yes - if the milestone was taken with a depth of 500 and we have a pruning delay that is set to 5000, then we only prune data that is 5500 milestones in the past. This allows us to create local snapshot files that are suitable for very fast syncs (new nodes) and still maintain enough old data to never run into issues with the coordinator suddenly approving stuff that is really old.

This comment has been minimized.

@karimodm

karimodm Sep 12, 2018

Member

Yep, that's clear; just to be more verbose in the doc.

This comment has been minimized.

@alon-e

alon-e Sep 12, 2018

Member

I agree that this description should be more informative.
what are the ranges, what do they mean for the users?

This comment has been minimized.

@hmoog

hmoog Sep 12, 2018

Contributor

Maybe also create a separate issue after merging so we can get these changes in first - its kind of tricky to break the whole shit down anyway

@alon-e
Copy link
Member

alon-e left a comment

looks good. agree w/ @karimodm - LOCAL_SNAPSHOTS_DEPTH can be more verbose.

@GalRogozinski GalRogozinski merged commit a76ea97 into iotaledger:dev Sep 13, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hmoog hmoog deleted the iotadevelopment:merge_config branch Sep 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment