Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent #1855

Closed
wants to merge 2 commits into from
Closed

ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent #1855

wants to merge 2 commits into from

Conversation

arshadmohammad
Copy link
Contributor

No description provided.

@symat
Copy link
Contributor

symat commented Apr 8, 2022

AFAIK your watcher should never be null. (Without a watcher you don't know if you are even connected to ZK or not... the successful connection and also the disconnection events reach the ZK client user through the watcher.) Previously we were throwing exception in this watcher==null case, and this was not not too bad, as most likely this is not how the caller app wanted to start the ZooKeeper client in the first place. Although NPE might not the right exception to throw :)

So my point here is: Isn't this change breaking compatibility? Shouldn't we throw at least some InvalidStateException after logging?

@eolivelli
Copy link
Contributor

I agree with Mate.
But maybe I am missing how to reproduce the problem.
We should also add a test that reproduces the problem

@arshadmohammad
Copy link
Contributor Author

I checked the zk client code carefully, NPE will occur only when watcher is set null either throw ZooKeeper constructor or through register method. Now I think we should do the exactly what had been submitted in latest ZOOKEEPER-1875.patch.

Updated the PR.

symat
symat approved these changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@symat symat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thank you!

symat
symat requested changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@symat symat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks some of our tests violate this rule. Can you take a look and maybe switch these to some noop watcher?

[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   QuorumDigestTest.testDigestMatchesDuringDiffSync:88->triggerSync:220->triggerOps:207 » IllegalArgument
[ERROR]   QuorumDigestTest.testDigestMatchesDuringSnapSync:96->triggerSync:220->triggerOps:207 » IllegalArgument
[ERROR]   QuorumDigestTest.testDigestMismatchesWhenTxnLost:173->triggerOps:207 » IllegalArgument
[ERROR]   ObserverMasterTest.testInOrderCommits:216 » IllegalArgument Invalid Watcher, s...
[ERROR]   ObserverMasterTest.testInOrderCommits:216 » IllegalArgument Invalid Watcher, s...
[INFO] 
[ERROR] Tests run: 2983, Failures: 0, Errors: 5, Skipped: 4

@arshadmohammad
Copy link
Contributor Author

looks some of our tests violate this rule. Can you take a look and maybe switch these to some noop watcher?

Corrected impacted test cases

symat
symat approved these changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@symat symat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!!

@asfgit asfgit closed this in 86690ff Apr 16, 2022
asfgit pushed a commit that referenced this pull request Apr 16, 2022
@arshadmohammad
ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.proces…
3f0810b 
…sEvent

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes #1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471 [Mohammad Arshad] Corrected impacted test cases
12f44d6 [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent

(cherry picked from commit 86690ff)
Signed-off-by: Mohammad Arshad <arshad@apache.org>
asfgit pushed a commit that referenced this pull request Apr 16, 2022
@arshadmohammad
ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.proces…
9769139 
…sEvent

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes #1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471 [Mohammad Arshad] Corrected impacted test cases
12f44d6 [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent

(cherry picked from commit 86690ff)
Signed-off-by: Mohammad Arshad <arshad@apache.org>
asfgit pushed a commit that referenced this pull request Apr 16, 2022
@arshadmohammad
ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.proces…
df5ee99 
…sEvent

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes #1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471 [Mohammad Arshad] Corrected impacted test cases
12f44d6 [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent

(cherry picked from commit 86690ff)
Signed-off-by: Mohammad Arshad <arshad@apache.org>
@anuragmadnawat1 anuragmadnawat1 mentioned this pull request Nov 1, 2022
Merged
anuragmadnawat1 pushed a commit to anuragmadnawat1/zookeeper that referenced this pull request Nov 1, 2022
@arshadmohammad @anurag-harness
ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.proces…
581ae1f 
…sEvent

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes apache#1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471 [Mohammad Arshad] Corrected impacted test cases
12f44d6 [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent
anuragmadnawat1 added a commit to anuragmadnawat1/zookeeper that referenced this pull request Nov 1, 2022
@anuragmadnawat1 @arshadmohammad
ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.proces…
fbfa557 
…sEvent (#56)

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes apache#1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471 [Mohammad Arshad] Corrected impacted test cases
12f44d6 [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent

Co-authored-by: Mohammad Arshad <arshad@apache.org>
anurag-harness pushed a commit to anurag-harness/zookeeper that referenced this pull request Jan 13, 2023
@arshadmohammad @anurag-harness
ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.proces…
03b9ac1 
…sEvent

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes apache#1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471 [Mohammad Arshad] Corrected impacted test cases
12f44d6 [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent
@anurag-harness anurag-harness mentioned this pull request Jan 13, 2023
Merged
anurag-harness added a commit to anurag-harness/zookeeper that referenced this pull request Jan 13, 2023
@anurag-harness @arshadmohammad
ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.proces…
06e9c16 
…sEvent (#23)

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes apache#1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471 [Mohammad Arshad] Corrected impacted test cases
12f44d6 [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent

Co-authored-by: Mohammad Arshad <arshad@apache.org>
desaikomal pushed a commit to linkedin/zookeeper that referenced this pull request Jun 17, 2023
@arshadmohammad @desaikomal
ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.proces…
79a6e55 
…sEvent

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes apache#1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471 [Mohammad Arshad] Corrected impacted test cases
12f44d6 [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent

(cherry picked from commit 86690ff)
Signed-off-by: Mohammad Arshad <arshad@apache.org>
desaikomal pushed a commit to linkedin/zookeeper that referenced this pull request Jun 27, 2023
@arshadmohammad @desaikomal
ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.proces…
ea8ae5e 
…sEvent

Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Mate Szalay-Beko <symat@apache.org>, Enrico Olivelli <eolivelli@apache.org>

Closes apache#1855 from arshadmohammad/ZOOKEEPER-1875-npe and squashes the following commits:

4a7d471 [Mohammad Arshad] Corrected impacted test cases
12f44d6 [Mohammad Arshad] ZOOKEEPER-1875: NullPointerException in ClientCnxn$EventThread.processEvent

(cherry picked from commit 86690ff)
Signed-off-by: Mohammad Arshad <arshad@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@symat symat symat approved these changes

@eolivelli eolivelli Awaiting requested review from eolivelli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@arshadmohammad @symat @eolivelli