-
Notifications
You must be signed in to change notification settings - Fork 1k
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
PHOENIX-6510 Double-Checked Locking field must be volatile #1264
Conversation
Change-Id: I4b5fd760605958d209409e740a6da2560e9d9f7a
Change-Id: I24ba028ccd1ca26ef4473338d6e7417244b2e389
Change-Id: Ie940b4abf58e9294859770b9a7542eb1d0a6e3d1
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.
+1 assuming tests pass
Something is wrong with the Jenkins. I tried to trigger the precommit job again but it failed outright immediately: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1264/2/console
|
I wonder if switching to eager initialization (at least in PhoenixConnection.java ) would be a more performant option. Looking at ConcurrentLinkedQueue, it seems to be cheap to initalize, and we could simplify the code as well. |
Yeah this lazy initialization in PhoenixConnection isn't used correctly anyway. Let's change it. Do you want it to be in a separate PR or are you okay to have it in the same one? |
Fine in this one. |
Change-Id: I39a954e9a6989b729e0c1c4849ab3bf5a6337a8b
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.
+1 LGTM
I'll wait for the Jenkins job to finish before merging. (If I forget it, ping me pls)
Jenkins is still down, it seems. |
I asked Infra to investigate INFRA-22106 and looks like it'll take a while for the hardware to reset. |
💔 -1 overall
This message was automatically generated. |
Thanks! |
No description provided.