Skip to content

Conversation

@kezhuw
Copy link
Member

@kezhuw kezhuw commented Jul 3, 2022

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.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Generally looks good. Perhaps we should add a curator-test-zk36 module for running tests with zookeeper 3.6.x to prevent regression? You may take a look at how curator-test-zk35 does now. I'm happy to review this patch :)

cc @kezhuw

`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.
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.

LGTM

@kezhuw kezhuw force-pushed the CURATOR-596-upgrade-zookeeper branch from e227575 to b83a475 Compare July 13, 2022 15:39
@kezhuw
Copy link
Member Author

kezhuw commented Jul 13, 2022

@tisonkun I have rebased this pr to master for 5.4.0-SNAPSHOT and push a fixup commit for module curator-test-zk36. Could you please take a look and trigger ci workflow ?

@tisonkun
Copy link
Member

@kezhuw Thanks for your contribution!

Merging if CI gives green.

@tisonkun
Copy link
Member

Merging...

@kezhuw thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants