Skip to content

ZOOKEEPER-3864: Reject create/renew/close global session in RO mode - #1381

Closed
jhuan31 wants to merge 1 commit into
apache:masterfrom
jhuan31:ZOOKEEPER-3864
Closed

ZOOKEEPER-3864: Reject create/renew/close global session in RO mode#1381
jhuan31 wants to merge 1 commit into
apache:masterfrom
jhuan31:ZOOKEEPER-3864

Conversation

@jhuan31

@jhuan31 jhuan31 commented Jun 16, 2020

Copy link
Copy Markdown

No description provided.

@jhuan31

jhuan31 commented Jun 16, 2020

Copy link
Copy Markdown
Author

looks like an infra issue. it failed only on s390x and the error msg is "An error occurred while generating the build script.An error occurred while generating the build script."

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

non blocking : might worth to add a test for this in ReadOnlyModeTest.

@jhuan31 jhuan31 closed this Jun 17, 2020
@jhuan31 jhuan31 reopened this Jun 17, 2020
@jhuan31

jhuan31 commented Jun 17, 2020

Copy link
Copy Markdown
Author

@hanm I think there is something wrong with ReadOnlyModeTest: some tests depend on renewing a global session, which is rejected by this change. The check result of the continuous integration is very confusing though--it doesn't show any test failure. Trying to fix it.

@jhuan31

jhuan31 commented Jun 17, 2020

Copy link
Copy Markdown
Author

Need to have ZOOKEEPER-3863, which replace SessionTrackerImpl with LearnerSessionTracker for ReadOnlyServer, to make this PR work coz SessionTrackerImpl can't whether a session is global or not.

@hanm

hanm commented Jun 18, 2020

Copy link
Copy Markdown
Contributor

Yes, ReadOnlyModeTest is failing because of this. #1380 doesn't seem to fix all the tests though - I tried to apply that to my local clone of this patch and all tests still failing. Let's fix all tests before landing this.

@hanm

hanm commented Jun 18, 2020

Copy link
Copy Markdown
Contributor

also, should we combine #1380 with this pull request? Logically, both are separate issues, but since this patch depends on PR1380 (which itself does not come with any unit test), I am thinking combine both in a single PR might be better (and probably easier to get all test pass).

@jhuan31

jhuan31 commented Jun 23, 2020

Copy link
Copy Markdown
Author

Combine this PR with #1380

@jhuan31 jhuan31 closed this Jun 23, 2020
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.

2 participants