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 a flake in the sync manager edge case test #5397

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

Stebalien
Copy link
Member

Depending on scheduling, we may see c2 before we see c1.

Depending on scheduling, we may see c2 before we see c1.
Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

LGTM I can't reproduce locally (after 1000's of runs) but this seems like it should fix the issue.

I'm deferring to @vyzo for the actual approval; accepting either c2 or c1 seems to defeat the purpose of the test altogether. I'm not sure how the done() API works but, as Sync() can in theory be run in parallel, to process tipsets in the proper order I suppose that this test is working under the assumption that syncManager is going to throttle them accordingly (coming from the same parent). If this is not the case then I have no idea what this test is doing and my green tick shouldn't count anyways.

@Stebalien
Copy link
Member Author

I believe the goal is that c1 should be the final decision, even if c1 comes first. This still tests that (ish).

Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

Looking at this again, c1 and c2 do not come from the same parent as I originally thought. Sync() calls will be issued in parallel for both, so this is in fact racy (not sure if by design). The fix makes sense as a temporary solution.

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

yes, that's the intention! thanks for fixing this.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

❤️

@magik6k magik6k linked an issue Jan 21, 2021 that may be closed by this pull request
@magik6k magik6k merged commit e5efe57 into master Jan 21, 2021
@magik6k magik6k deleted the fix/sync-manager-test-flake branch January 21, 2021 09:18
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.

TestSyncManagerEdgeCase is flaky
4 participants