Move HighWaterMark to the top of the struct in order to fix arm #33117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-github-robot
added
size/S
lavalamp
reviewed
Sep 20, 2016
| @@ -61,6 +61,12 @@ func exceptKey(except string) includeFunc { | |||
|
|
|||
| // etcdWatcher converts a native etcd watch to a watch.Interface. | |||
| type etcdWatcher struct { | |||
| // HighWaterMarks for performance debugging. | |||
| // Important: Since HighWaterMark is using sync/atomic, it has to be at the top of the struct due to a bug on 32-bit platforms | |||
| // See: https://golang.org/pkg/sync/atomic/ for more information | |||
|
lgtm, but seriously, this is terrible, we probably have this bug all over. And what if someone embeds the etcdWatcher struct in something else not at the top? We need the compiler to enforce things like this, it just can't be done manually. Can you file or link a golang issue for this? |
lavalamp
added
lgtm
|
Automatic merge from submit-queue |
k8s-github-robot
pushed a commit
that referenced
this pull request
Sep 23, 2016
Automatic merge from submit-queue Move HighWaterMark to the top of the struct in order to fix arm, second time ref: #33117 Sorry for not fixing everyone at once, but I seriously wasn't prepared for that quick LGTM 😄, so here's the other half. @lavalamp > lgtm, but seriously, this is terrible, we probably have this bug all over. And what if someone embeds the etcdWatcher struct in something else not at the top? We need the compiler to enforce things like this, it just can't be done manually. Can you file or link a golang issue for this? I totally agree! There isn't currently a way of programmatically detecting this unfortunately. I guess @davecheney or @minux can explain better to you why it's so hard. This is noted in https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/multi-platform.md as a corner case indeed. @pwittrock This should be cherrypicked toghether with #33117
|
Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
jessfraz
added
release-note
shyamjvs
pushed a commit
to shyamjvs/kubernetes
that referenced
this pull request
Dec 1, 2016
…of-#33117-kubernetes#33376-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of kubernetes#33117 kubernetes#33376 Cherry pick of kubernetes#33117 kubernetes#33376 on release-1.4.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I haven't tested this yet, but let's see how e2e tests react.
It should be no difference at all except for that it will fix arm.
etcd has had to do this some times (and I think there are some fixes like this that are needed for etcd as well)
For reference see: https://golang.org/pkg/sync/atomic/
This should be a cherrypick-candidate for v1.4.1 (as I understand it, v1.4.0 has clearly left the cherrypickable state)
@lavalamp @pwittrock @xiang90 @smarterclayton
This change is