Skip to content

Conversation

bprashanth
Copy link
Contributor

Make TestRCExpectations (#7860) use LoadInt. I think the watch failure is just a victim here because it needs to start multiple goroutines and their timings didn't quite line up within 100ms. It already uses a fake listwatcher, so suggestions other than just increasing its timeout are welcome.

@@ -124,9 +124,6 @@ func (r *RCExpectations) ExpectDeletions(rc *api.ReplicationController, dels int
// Decrements the expectation counts of the given rc.
func (r *RCExpectations) lowerExpectations(rc *api.ReplicationController, add, del int) {
if podExp, exists, err := r.GetExpectations(rc); err == nil && exists {
if podExp.add > 0 && podExp.del > 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this isn't worth the loadint.
what can go wrong here, is go just being panicky? im ok with these being inaccurate as i'm just using them for logging. The memory isn't goint to get deallocated because these are members in a struct I have a ref to right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the memory won't get deallocated. It's complaining because it's technically a race (imagine a system where 64 bit ints took two words).

@lavalamp
Copy link
Contributor

lavalamp commented May 6, 2015

Fixes #7860

LGTM

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2015
lavalamp added a commit that referenced this pull request May 7, 2015
@lavalamp lavalamp merged commit 1177adf into kubernetes:master May 7, 2015
@bprashanth bprashanth deleted the testWatchControllers branch October 26, 2015 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants