HADOOP-17612. Upgrade Zookeeper to 3.6.3 and Curator to 5.2.0 #5067
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
…#3241) Signed-off-by: Akira Ajisaka <[email protected]>
…Cache / TreeCache (apache#3266) Signed-off-by: Akira Ajisaka <[email protected]>
|
💔 -1 overall
This message was automatically generated. |
apache#2047) Improving router's performance for delegation tokens related operations. It achieves the goal by removing watchers from router on tokens since based on our experience. The huge number of watches inside Zookeeper is degrading Zookeeper's performance pretty hard. The current limit is about 1.2-1.5 million.
|
💔 -1 overall
This message was automatically generated. |
…recommended methods. (apache#4812)
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Please check if the failed tests are relevant. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Can you review the change and latest build to see if anything else to fix? Once |
|
confirmed that these 4 checkstyle complaints also happen in trunk. Not related with this PR. |
xinglin
reviewed
Oct 28, 2022
| @@ -417,24 +408,18 @@ private void processTokenAddOrUpdate(byte[] data) throws IOException { | |||
| if (numRead > -1) { | |||
| DelegationTokenInformation tokenInfo = | |||
| new DelegationTokenInformation(renewDate, password); | |||
| synchronized (this) { | |||
There was a problem hiding this comment.
This change removes a few synchronized keywords. I cross-check with trunk and trunk also removes these. seems fine but I have no context on why synchronized was used before and now removed.
There was a problem hiding this comment.
It was introduced from https://github.com/apache/hadoop/pull/2047/files
I don't get the full context of that change but in AbstractDelegationTokenSecretManager.java, they changed implementations of currentTokens and allKeys from hashmap (not thread safe) to concurrentHashMap(thread safe). That's why they removed synchronized as it's a duplicate protection.
| @@ -101,6 +101,7 @@ public void testMultiNodeOperationWithoutWatch() throws Exception { | |||
| } | |||
| } | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
There was a problem hiding this comment.
trunk does not suppress here. Maybe we shouldn't neither?
| @@ -162,6 +162,7 @@ public void testMultiNodeTokenRemovalShortSyncWithoutWatch() | |||
|
|
|||
| // This is very unlikely to happen in real case, but worth putting | |||
| // the case out | |||
| @SuppressWarnings("unchecked") | |||
There was a problem hiding this comment.
trunk does not have suppress here.
There was a problem hiding this comment.
Yes it was added by me to fix failed javac problem reported in earlier builds. There're two similar places(line135&201) and either will fail javac.
So two ways:
- Keep it in sync with trunk and let javac fail for this PR.
- (What I did here) fix it by adding suppress warning here as I'm pretty confident those types are correct. This test file is inconsistent - you can see the untouched method testMultiNodeOperationWithoutWatch() has the exact same problem and has suppress warning. So I think it's applicable to the other two methods as well.
There was a problem hiding this comment.
I agree with you. Either way is fine. Will defer to @omalley for which approach to take on here.
|
fyi, created a PR for fixing |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
4 tasks
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.
Description of PR
This includes cherry-picks from commit
ccfa072,23e2a0b,84110d8,86b84ed,0db3ee5in trunk to upgrade zookeeper and curator.It's a nested change but every cherry pick is clean.
ccfa072#324123e2a0b#326684110d8#204786b84ed#48850db3ee5#4812Two minor modifications are adding suppress warning annotation in
testMultiNodeTokenRemovalShortSyncWithoutWatch()andtestMultiNodeTokenRemovalLongSyncWithoutWatch()methods inTestZKDelegationTokenSecretManagerImpl. See comment .How was this patch tested?
mvn clean install -Pdist -Dtar -DskipTests -Dmaven.javadoc.skip builds successfully
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?