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

Fix nodes coordinator registry save #3504

Merged
merged 6 commits into from
Oct 15, 2021

Conversation

AdoAdoAdo
Copy link
Contributor

In case there is a restart in every epoch, the registry for nodes coordinator does not get pruned.
This PR ensures that every nodes coordinator registry save is done only for the configured number of epochs.

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #3504 (9d65912) into development (ae292a4) will increase coverage by 0.00%.
The diff coverage is 90.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #3504   +/-   ##
============================================
  Coverage        73.87%   73.88%           
============================================
  Files              582      582           
  Lines            74532    74545   +13     
============================================
+ Hits             55064    55075   +11     
- Misses           15065    15066    +1     
- Partials          4403     4404    +1     
Impacted Files Coverage Δ
sharding/indexHashedNodesCoordinatorWithRater.go 80.39% <0.00%> (-3.29%) ⬇️
sharding/indexHashedNodesCoordinator.go 83.30% <100.00%> (ø)
sharding/indexHashedNodesCoordinatorRegistry.go 73.82% <100.00%> (+3.53%) ⬆️
process/block/baseProcess.go 68.46% <0.00%> (-0.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 945f104...9d65912. Read the comment docs.

raduchis
raduchis previously approved these changes Oct 12, 2021

// LoadState loads the nodes coordinator state from the used boot storage
func (ihgs *indexHashedNodesCoordinatorWithRater) LoadState(key []byte) error {
return ihgs.baseLoadState(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for the move to the appropriate file

}

for epoch, epochNodesData := range ihgs.nodesConfig {
minEpoch := 0
if ihgs.currentEpoch >= nodeCoordinatorStoredEpochs {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a typo here? I think nodesCoordinatorStoredEpochs sounds better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, for performance reasons I would have put it in reverse, like:

minEpoch = int(ihgs.currentEpoch) - nodeCoordinatorStoredEpochs + 1
if minEpoch < 0{
    minEpoch = 0
}

as for the current state (epoch 400+) the node will always execute the if branch

iulianpascalau
iulianpascalau previously approved these changes Oct 12, 2021
Copy link
Contributor

@iulianpascalau iulianpascalau 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 on internal tests: only epochs 7, 6 and 5 were saved in the storage.

Copy link
Collaborator

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

System test passed.
image

@LucianMincu LucianMincu merged commit e958ed3 into development Oct 15, 2021
@LucianMincu LucianMincu deleted the fix-nodes-coordinator-registry-pruning branch October 15, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants