-
Notifications
You must be signed in to change notification settings - Fork 14.8k
MINOR: Ensure proper visibility of attribute accesses across threads #21014
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
Conversation
lianetm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM.
chia7712
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @dajac would you mind also backporting this change to 4.1? I tried to cherry-pick it myself but it failed
| * This is initialised when the {@link GroupCoordinator#onNewMetadataImage(CoordinatorMetadataImage, CoordinatorMetadataDelta)} is called | ||
| */ | ||
| private CoordinatorMetadataImage metadataImage = null; | ||
| private volatile CoordinatorMetadataImage metadataImage = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume those changes are bug fixes, and the 4.1 branch may have a next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems trunk is missing this fix. @majialong could you file a minor patch for trunk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open #21021 for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trunk is covered by #21008.
|
open #21020 for the backport |
…21020) backport #21014 Reviewers: Chia-Ping Tsai <[email protected]>
|
Sorry guys. I wanted to follow-up on this on Monday (due to the weekend). You got there before I could. Thanks! |
…21023) backport #21014. In 4.0, `MetadataImage` has not yet been introduced into `GroupCoordinatorService`. Therefore, this patch only includes changes to `CoordinatorRuntime`. Reviewers: Chia-Ping Tsai <[email protected]>
I found those two cases while working in this area. In both cases, they
are read by multiple threads so they must either be volatile or
acquiring the appropriate lock. Using volatile is fine here.
Reviewers: Lianet Magrans [email protected]