Skip to content
This repository was archived by the owner on Aug 23, 2020. It is now read-only.

Feat: added config variables for local snapshots#981

Merged
GalRogozinski merged 1 commit intoiotaledger-archive:devfrom
iotadevelopment:merge_config
Sep 13, 2018
Merged

Feat: added config variables for local snapshots#981
GalRogozinski merged 1 commit intoiotaledger-archive:devfrom
iotadevelopment:merge_config

Conversation

@hmoog
Copy link
Copy Markdown

@hmoog 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).";

Copy link
Copy Markdown
Contributor

@karimodm karimodm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@karimodm karimodm Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.";
Copy link
Copy Markdown
Contributor

@karimodm karimodm Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@alon-e alon-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@GalRogozinski GalRogozinski merged commit a76ea97 into iotaledger-archive:dev Sep 13, 2018
@hmoog hmoog deleted the merge_config branch September 13, 2018 15:16
@f-ben f-ben mentioned this pull request Mar 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants