Skip to content

ZOOKEEPER-3863: Do not track global sessions in ReadOnlyZooKeeperServer #1380

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
Closed
jhuan31 wants to merge 4 commits into from
Closed

ZOOKEEPER-3863: Do not track global sessions in ReadOnlyZooKeeperServer #1380

jhuan31 wants to merge 4 commits into from

Conversation

@jhuan31
Copy link

@jhuan31 jhuan31 commented Jun 15, 2020

No description provided.

hanm
hanm approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@hanm hanm left a comment

Choose a reason for hiding this comment

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

LGTM.

We could also just make ReadOnlyZooKeeperServer extends LearnerZooKeeperServer as opposed to extend ZooKeeperServer, so we can reuse the LearnerSessionTracker without customizing ReadOnlyZooKeeperServer. We also need add the start to the SessionTracker interface to avoid casting all over the place. Anyway, seems some mess that can be cleaned up a little bit over the code base regarding session handling, let's do that separately.

@hanm hanm mentioned this pull request Jun 18, 2020
Closed
@jhuan31
Copy link
Author

jhuan31 commented Jun 23, 2020

This PR now includes ZOOKEEPER-3863 and ZOOKEEPER-3864 and updated unit tests.

@jhuan31 jhuan31 closed this Jun 23, 2020
@jhuan31 jhuan31 reopened this Jun 23, 2020
@jhuan31
Copy link
Author

jhuan31 commented Jun 23, 2020

can't figure out what's wrong with the build. The only msg is "An error occurred while generating the build script."

hanm
hanm approved these changes Jun 24, 2020
View reviewed changes
Copy link
Contributor

@hanm hanm left a comment

Choose a reason for hiding this comment

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

lgtm. ran this patch locally and all test pass.

@hanm
Copy link
Contributor

hanm commented Jun 24, 2020

retest maven build

1 similar comment
@hanm
Copy link
Contributor

hanm commented Jun 24, 2020

retest maven build

eolivelli
eolivelli approved these changes Jun 24, 2020
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Looks good

@eolivelli
Copy link
Contributor

eolivelli commented Jun 24, 2020

I have triggered Travis

@hanm
Copy link
Contributor

hanm commented Jun 24, 2020

Making good progress now, with Travis green :)

Jenkins is consistently failing with C client read-only tests:

[exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc:100: Assertion: equality assertion failed [Expected: 0, Actual : -7] [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc:100: Assertion: equality assertion failed [Expected: 0, Actual : -7] [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/tests/TestSASLAuth.cc:214: Assertion: assertion failed [Expression: ctx.waitForConnected(zk)] [exec] Failures !!! [exec] Run: 88 Failure total: 3 Failures: 3 Errors: 0

@jhuan31 I think we need update C read only test similar like we did for Java test to explicit set/enable local session in test, because we are now rejecting global session for read only servers.

@hanm
Copy link
Contributor

hanm commented Jun 24, 2020

I did a super quick look on the C test, looks like no easy way to programmatically enable local session on the ZooKeeperQuorumServer. One approach is to create a read only test specific zoo.cfg and enable local session in the config file, instead of doing it programmatically.

@jhuan31
Copy link
Author

jhuan31 commented Jul 1, 2020

ah, didn't know that C test needs to be updated too. taking a look.

@hanm
Copy link
Contributor

hanm commented Sep 13, 2020

retest maven build

@hanm
Copy link
Contributor

hanm commented Sep 15, 2020

@jhuan31 The latest pre-merge job pass - looks like we don't have any issues on C client side. I also verified this pull request locally (CentOS) and all C client test pass. Not sure why the previous pre-merge job gives specific errors on read-only related test failures.

@eolivelli Since we have a green build (pre-merge) now, should we commit this change? Both of us is +1 on this. I hope I can trigger a JenkinsMaven build for this but I don't know how - a green pre-merge build seems enough.

This will be a very valuable additions to ZooKeeper as it fixes ReadOnly mode, which is super useful for service discovery use cases.

@eolivelli
Copy link
Contributor

eolivelli commented Sep 16, 2020

@hanm yes we can commit it
I will take care of that today

@eolivelli eolivelli closed this in c47ef90 Sep 17, 2020
@hanm hanm mentioned this pull request Oct 1, 2020
Open
kezhuw added a commit to kezhuw/curator that referenced this pull request Jul 3, 2022
@kezhuw
CURATOR-596: Upgrade ZooKeeper to 3.7.1
a88ad38 
`TestReadOnly` fails due to ZOOKEEPER-3863(apache/zookeeper#1380).

ZOOKEEPER-3863 uses `QuorumPeerConfig.localSessionsEnabled` to toggle
local sessions supports in `ReadOnlyZooKeeperServer`.

This pr populates `QuorumPeerConfig.localSessionsEnabled` and
`QuorumPeerConfig.localSessionsUpgradingEnabled` from system property
`readonlymode.enabled` for simplicity.
@kezhuw kezhuw mentioned this pull request Jul 3, 2022
Merged
kezhuw added a commit to kezhuw/curator that referenced this pull request Jul 3, 2022
@kezhuw
CURATOR-596: Upgrade ZooKeeper to 3.7.1
e227575 
`TestReadOnly` fails due to ZOOKEEPER-3863(apache/zookeeper#1380).

ZOOKEEPER-3863 uses `QuorumPeerConfig.localSessionsEnabled` to toggle
local sessions supports in `ReadOnlyZooKeeperServer`.

This pr populates `QuorumPeerConfig.localSessionsEnabled` and
`QuorumPeerConfig.localSessionsUpgradingEnabled` from system property
`readonlymode.enabled` for simplicity.
kezhuw added a commit to kezhuw/curator that referenced this pull request Jul 13, 2022
@kezhuw
CURATOR-596: Upgrade ZooKeeper to 3.7.1
85513cd 
`TestReadOnly` fails due to ZOOKEEPER-3863(apache/zookeeper#1380).

ZOOKEEPER-3863 uses `QuorumPeerConfig.localSessionsEnabled` to toggle
local sessions supports in `ReadOnlyZooKeeperServer`.

This pr populates `QuorumPeerConfig.localSessionsEnabled` and
`QuorumPeerConfig.localSessionsUpgradingEnabled` from system property
`readonlymode.enabled` for simplicity.
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
@RokLenarcic
ZOOKEEPER-3863: Do not track global sessions in ReadOnlyZooKeeperServer
debd545 
Author: Jie Huang <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Michael Han <[email protected]>

Closes apache#1380 from jhuan31/ZOOKEEPER-3863
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
@RokLenarcic
ZOOKEEPER-3863: Do not track global sessions in ReadOnlyZooKeeperServer
824e9d8 
Author: Jie Huang <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Michael Han <[email protected]>

Closes apache#1380 from jhuan31/ZOOKEEPER-3863
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
@RokLenarcic
ZOOKEEPER-3863: Do not track global sessions in ReadOnlyZooKeeperServer
ca94838 
Author: Jie Huang <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Michael Han <[email protected]>

Closes apache#1380 from jhuan31/ZOOKEEPER-3863
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
@RokLenarcic
ZOOKEEPER-3863: Do not track global sessions in ReadOnlyZooKeeperServer
e6197de 
Author: Jie Huang <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Michael Han <[email protected]>

Closes apache#1380 from jhuan31/ZOOKEEPER-3863
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hanm hanm hanm approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jhuan31 @hanm @eolivelli