Skip to content

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

Closed
jhuan31 wants to merge 4 commits into
apache:masterfrom
jhuan31:ZOOKEEPER-3863
Closed

ZOOKEEPER-3863: Do not track global sessions in ReadOnlyZooKeeperServer#1380
jhuan31 wants to merge 4 commits into
apache:masterfrom
jhuan31:ZOOKEEPER-3863

Conversation

@jhuan31

@jhuan31 jhuan31 commented Jun 15, 2020

Copy link
Copy Markdown

No description provided.

hanm
hanm approved these changes Jun 18, 2020
View reviewed changes

@hanm hanm left a comment

Copy link
Copy Markdown
Contributor

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

jhuan31 commented Jun 23, 2020

Copy link
Copy Markdown
Author

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

jhuan31 commented Jun 23, 2020

Copy link
Copy Markdown
Author

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

@hanm hanm left a comment

Copy link
Copy Markdown
Contributor

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

hanm commented Jun 24, 2020

Copy link
Copy Markdown
Contributor

retest maven build

1 similar comment
@hanm

hanm commented Jun 24, 2020

Copy link
Copy Markdown
Contributor

retest maven build

eolivelli
eolivelli approved these changes Jun 24, 2020
View reviewed changes

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

@eolivelli

eolivelli commented Jun 24, 2020

Copy link
Copy Markdown
Contributor

I have triggered Travis

@hanm

hanm commented Jun 24, 2020

Copy link
Copy Markdown
Contributor

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

hanm commented Jun 24, 2020

Copy link
Copy Markdown
Contributor

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

jhuan31 commented Jul 1, 2020

Copy link
Copy Markdown
Author

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

@hanm

hanm commented Sep 13, 2020

Copy link
Copy Markdown
Contributor

retest maven build

@hanm

hanm commented Sep 15, 2020

Copy link
Copy Markdown
Contributor

@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

eolivelli commented Sep 16, 2020

Copy link
Copy Markdown
Contributor

@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
Jie Huang @RokLenarcic
ZOOKEEPER-3863: Do not track global sessions in ReadOnlyZooKeeperServer
debd545 
Author: Jie Huang <jiehuang@fb.com>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Michael Han <hanm@apache.org>

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

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Michael Han <hanm@apache.org>

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

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Michael Han <hanm@apache.org>

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

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Michael Han <hanm@apache.org>

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