Skip to content
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

Refactor snapshotting mode - Closes #1949, #1927, #1869, #1868, #1933 #1963

Merged
merged 34 commits into from May 3, 2018

Conversation

Projects
3 participants
@4miners
Copy link
Member

commented Apr 27, 2018

What was the problem?

As described in corresponding issues.

How did I fix it?

Refacored snapshotting logic.

How to test it?

  • Execute loader module unit tests.
  • Run functional system tests for snapshotting.

Review checklist

  • The PR closes #1949, closes #1927, closes #1869, closes #1868, closes #1933
  • All new code is covered with unit tests
  • All new code was formatted with Prettier
  • Linting passes
  • Tests pass
  • Commit messages follow the commit guidelines
  • Documentation has been added/updated

@4miners 4miners added this to New Issues in Sprint Board 23-04-18 via automation Apr 27, 2018

@4miners 4miners requested a review from nazarhussain Apr 27, 2018

@MaciejBaj MaciejBaj added this to Open Issues in Version 1.0.0 via automation Apr 27, 2018

* @returns {Promise}
* @todo Add description for the params and the return value
*/
truncateBlocks(height) {

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

The name refers to truncating blocks for a specific height while the behavior is to truncate all blocks with above that height so we should rename this method appropriately.

* @todo Add description for the params and the return value
*/
truncateBlocks(height) {
return this.db.none(sql.truncateBlocks, [height]);

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

Though out our db layer, we decide to use only named params instead of $1 and $2 etc. And we are fixing all the code which ever came under refactoring cycle. Pleas update this to code as well to use named params.

@@ -99,7 +100,6 @@ module.exports = {
restoreVotesSnapshot: link('rounds/restore_votes_snapshot.sql'),
getMemRounds: link('rounds/get_mem_rounds.sql'),
flush: link('rounds/flush.sql'),
truncateBlocks: link('rounds/truncate_blocks.sql'),

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

I like it moving to concerned db repo. 👍

@@ -103,7 +103,7 @@ function Config(packageJson) {
}

if (program.snapshot) {
appConfig.loading.snapshot = Math.abs(Math.floor(program.snapshot));
appConfig.loading.snapshot = program.snapshot;

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

The name snapshot tends to think of it as boolean value. Would be more nice if we rename it to snapshotRound that gave more meanings to code.

@@ -403,7 +401,7 @@ __private.loadBlockChain = function() {
modules.blocks.process.loadBlocksOffset(
limit,
offset,
verify,
true,

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

👍

let targetRound = Math.floor(height / constants.activeDelegates);
targetRound = isNaN(library.config.loading.snapshot)
? targetRound
: Math.min(targetRound, library.config.loading.snapshot);

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

See here renaming snapshot to snapshotRound will gave good meanings to this code.

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

If we create two variables, that will clarify code further.

const snapshotRound = library.config.loading.snapshot;
const totalRounds = Math.floor(height / constants.activeDelegates);
const targetRound = isNaN(snapshotRound) ? totalRounds : Math.min(totalRounds, snapshotRound);
`Snapshotting to end of round: ${targetRound}, height: ${targetHeight}`
);

let offset = 1;

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

Rename it to currentHeight or activeHeight

},
loadBlocksOffset(seriesCb) {
async.until(
() => targetHeight < offset,

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

Change above will make more sense with current statement.

() => targetHeight < currentHeight,
loadBlocksOffset(seriesCb) {
async.until(
() => targetHeight < offset,
cb => {

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

This cb should be renamed to untilCb or asyncUntilCb as per our perviously discussed approached to name callbacks.

constants.activeDelegates,
offset,
true,
err => {

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain Apr 27, 2018

Contributor

This err should be renamed as loadBlocksOffsetErr to make clear code and to avoid any error shadowing.

@MaciejBaj MaciejBaj moved this from New Issues to Open PRs in Sprint Board 23-04-18 Apr 27, 2018

@MaciejBaj MaciejBaj moved this from Open Issues to Open PRs in Version 1.0.0 May 2, 2018

@4miners 4miners requested a review from nazarhussain May 2, 2018

@4miners

This comment has been minimized.

Copy link
Member Author

commented May 2, 2018

Resolved comments from review, added unit tests. Please review again @nazarhussain 😄

@4miners 4miners changed the title Refactor snapshotting mode - Closes #1949, #1927, #1869, #1868 Refactor snapshotting mode - Closes #1949, #1927, #1869, #1868, #1933 May 2, 2018

)}, height: ${currentHeight}`
);
modules.blocks.process.loadBlocksOffset(
constants.activeDelegates,

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain May 2, 2018

Contributor

@4miners We have a config config.loading.loadPerIteration which where used before as the limit. Is it a specific requirement to change it to constants.activeDelegates

This comment has been minimized.

Copy link
@4miners

4miners May 2, 2018

Author Member

We need to always process only 1 round of blocks (101), so we can end snapshotting exactly after round ended.

.deleteBlocksAfterHeight(targetHeight)
.then(() => {
seriesCb();
})

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain May 2, 2018

Contributor

You can simplify it by .then(seriesCb) as deleteBlocksAfterHeight does not return any data.

library.logger.info('Snapshot creation finished');
}
process.emit('cleanup');
};

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain May 2, 2018

Contributor

Why it was necessary to move this code block outside the async.series final call back? only for testing?

With use of async.series in createSnapshot function the whole code flow converted to synchronous. Then I don't think its a good idea to trigger an event outside synchronous flow for few code lines. Specially when these code lines can only be triggered from one location.

}).then(() => {
return getMemAccounts().then(_accounts => {
// Save copy of mem_accounts table
memAccountsBeforeSnapshot = _.cloneDeep(_accounts);

This comment has been minimized.

Copy link
@nazarhussain

nazarhussain May 2, 2018

Contributor

Does it contains all mem_accounts2... tables as well?

This comment has been minimized.

Copy link
@4miners

4miners May 2, 2018

Author Member

No. It's basic scenario to ensure that snapshotting process ends exactly when it should (so just after round is finished).

@MaciejBaj MaciejBaj requested a review from SargeKhan May 2, 2018

@MaciejBaj MaciejBaj merged commit ec3e469 into 1.0.0-beta.8 May 3, 2018

1 of 3 checks passed

jenkins-ci/lisk-core This commit is being built
Details
jenkins-ci/lisk-core-integration This commit is being built
Details
security/snyk - package.json No dependency changes
Details

Version 1.0.0 automation moved this from Open PRs to Merged PRs May 3, 2018

Sprint Board 23-04-18 automation moved this from Open PRs to Merged PRs May 3, 2018

@MaciejBaj MaciejBaj deleted the snapshotting branch May 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.