You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/11/02 05:27:08 UTC
[GitHub] [zookeeper] cnauroth opened a new pull request, #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
cnauroth opened a new pull request, #1942:
URL: https://github.com/apache/zookeeper/pull/1942
…ntics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] cnauroth merged pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
cnauroth merged PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] cnauroth commented on pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
cnauroth commented on PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942#issuecomment-1301683143
I have merged this to master, branch-3.8 and branch-3.7. @eolivelli , thank you for reviewing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942#issuecomment-1299635251
I think that we should port this to the active branches, at least 3.8
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] cnauroth commented on pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
cnauroth commented on PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942#issuecomment-1301517081
There was one test failure, which appears to be unrelated.
```
[ERROR] Tests run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 25.109 s <<< FAILURE! - in org.apache.zookeeper.server.ConnectionMetricsTest
[ERROR] testRevalidateCount Time elapsed: 1.195 s <<< ERROR!
java.lang.NullPointerException
at org.apache.zookeeper.test.QuorumUtil.getConnectionStringForServer(QuorumUtil.java:343)
at org.apache.zookeeper.server.ConnectionMetricsTest.testRevalidateCount(ConnectionMetricsTest.java:65)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] sonatype-lift[bot] commented on pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942#issuecomment-1299601080
:warning: **52 God Classes** were detected by Lift in this project. [Visit the Lift web console](https://lift.sonatype.com/results/github.com/apache/zookeeper/01GGVCXE6YD5J4DMW6NQ80YQMF?tab=technical-debt&utm_source=github.com&utm_campaign=lift-comment&utm_content=apache\%20zookeeper) for more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] cnauroth commented on pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
cnauroth commented on PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942#issuecomment-1299598419
@eolivelli , @symat or @anmolnar , would you please review?
As discussed in JIRA issue [ZOOKEEPER-4460](https://issues.apache.org/jira/browse/ZOOKEEPER-4460), the ZooKeeper codebase currently contains an override of `Thread#getId()`, which will be incompatible with OpenJDK Project Loom, targeted to Java 19. I've taken the approach of renaming `QuorumPeer#getId()` to `QuorumPeer#getMyId()`, consistent with documented terminology for the ID assigned to a peer in a quorum. From what I can tell, it was never intentional to override `Thread#getId()`. It was just a logical choice for a method name in a class that coincidentally also `extends Thread`.
`QuorumPeer` is a server-side class that isn't part of a public API, so we don't need to preserve backward compatibility. As a safety measure, I reviewed the code for [Apache Curator](https://github.com/apache/curator), where I suspected there might be a deeper integration with ZooKeeper internals for testing infrastructure. Even in Curator, I didn't find any calls to `QuorumPeer#getId()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] cnauroth commented on pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
cnauroth commented on PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942#issuecomment-1299612003
The sonatype-lift warnings are potentially interesting, but unrelated to code changed by this patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942#discussion_r1011213870
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java:
##########
@@ -510,12 +510,12 @@ private boolean startConnection(Socket sock, Long sid) throws IOException {
}
// If lost the challenge, then drop the new connection
- if (sid > self.getId()) {
- LOG.info("Have smaller server identifier, so dropping the connection: (myId:{} --> sid:{})", self.getId(), sid);
+ if (sid > self.getMyId()) {
+ LOG.info("Have smaller server identifier, so dropping the connection: (myId:{} --> sid:{})", self.getMyId(), sid);
Review Comment:
*RESOURCE_LEAK:* resource of type `java.io.DataInputStream` acquired by call to `new()` at line 498 is not released after line 514.
---
<details><summary><b>ℹ️ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=350061542&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=350061542&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350061542&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350061542&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=350061542&lift_comment_rating=5) ]
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java:
##########
@@ -658,13 +658,13 @@ void lead() throws IOException, InterruptedException {
// us. We do this by waiting for the NEWLEADER packet to get
// acknowledged
- waitForEpochAck(self.getId(), leaderStateSummary);
+ waitForEpochAck(self.getMyId(), leaderStateSummary);
Review Comment:
💬 2 similar findings have been found in this PR
---
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `Leader.lead()` indirectly reads with synchronization from `this.leaderStateSummary`. Potentially races with unsynchronized write in method `Leader.lead()`.
Reporting because this access may occur on a background thread.
---
<details><summary><b>🔎 Expand here to view all instances of this finding</b></summary><br/>
<div align="center">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java | [606](https://github.com/cnauroth/zookeeper/blob/d3879db1ed019892373683694663b05ade0db6fe/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L606)|
| zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java | [667](https://github.com/cnauroth/zookeeper/blob/d3879db1ed019892373683694663b05ade0db6fe/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L667)|
<p><a href="https://lift.sonatype.com/results/github.com/apache/zookeeper/01GGVCXE6YD5J4DMW6NQ80YQMF?t=Infer|THREAD_SAFETY_VIOLATION" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary><b>ℹ️ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=350061670&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=350061670&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350061670&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=350061670&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=350061670&lift_comment_rating=5) ]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] cnauroth commented on pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
cnauroth commented on PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942#issuecomment-1301489569
Thanks for the review, @eolivelli ! I've confirmed that this patch backports cleanly to branch-3.8 and branch-3.7, so I'll plan on committing to those branches. (Going back any further than that will hit merge conflicts.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on pull request #1942: ZOOKEEPER-4460: QuorumPeer overrides Thread.getId with different sema…
Posted by GitBox <gi...@apache.org>.
li4wang commented on PR #1942:
URL: https://github.com/apache/zookeeper/pull/1942#issuecomment-1308116154
The test failure `ConnectionMetricsTest.testRevalidateCount` is related to the changes in this PR.
` int follower1 = (int) util.getFollowerQuorumPeers().get(0).getId()`
The test calls getId() to get the server id. Since getId() was changed to getMyId, the super class Thread.getId() is called and the thread id was returned. There is no corresponding peer associated to the thread id, that's why NPE is thrown.
The test passed after changing getId() to getMyId(). I will open a JIRA to check in the fix. It would great if @eolivelli can do a quick review and get it merged. Thanks.
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/test/java/org/apache/zookeeper/server/ConnectionMetricsTest.java#L60-L65
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org