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

storage: fix bug for the testrace of TestSystemZoneConfigs #19208

Merged
merged 1 commit into from
Oct 12, 2017
Merged

storage: fix bug for the testrace of TestSystemZoneConfigs #19208

merged 1 commit into from
Oct 12, 2017

Conversation

a6802739
Copy link
Contributor

@a6802739 a6802739 commented Oct 12, 2017

Fixes #19180.
Fixes #19207.

@a-robinson. PLAT.

I have run make testrace for this test about 10 times, and it seems ok.

The main problem I think is when we try to update desc.Replicas = append(desc.Replicas, newReplica), it will race with other goroutines, so I make a copy for rangeInfo.Desc.Replicas.

@a6802739 a6802739 requested a review from a team October 12, 2017 09:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@a6802739 a6802739 requested a review from a-robinson October 12, 2017 09:15
@tbg
Copy link
Member

tbg commented Oct 12, 2017

Ah, hadn't seen this -- I just independently opened a PR with a fix: https://github.com/cockroachdb/cockroach/pull/19212/files

Mind using the code from my PR (since I think it's a little cleaner) and add Fixes #19207. to your commit message, and then we can merge this and I close mine?

BTW, I think the Fixes XYZ must be on its own line, so like this:

Fixes #19207.
Fixes #19180.

@a6802739
Copy link
Contributor Author

@tschottdorf, Thank you very much.

Yes, I have seen your code, It's definitely much cleaner.

And Thanks for your kind remind about Fixes, I have changed it, It looks much better now. :)

@tbg tbg changed the title storage : fix bug for the testrace of TestSystemZoneConfigs storage: fix bug for the testrace of TestSystemZoneConfigs Oct 12, 2017
@tbg
Copy link
Member

tbg commented Oct 12, 2017

Looks good! Merge whenever you're ready.

Copy link
Contributor

@a-robinson a-robinson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @a6802739!

Looks good! Merge whenever you're ready.

Did you just LGTM your own code? 🤔

@a6802739 a6802739 merged commit f0a3407 into cockroachdb:master Oct 12, 2017
@a6802739 a6802739 deleted the TestSystemZoneConfigs branch October 12, 2017 14:10
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.

4 participants