Skip to content

Conversation

@jhuan31
Copy link

@jhuan31 jhuan31 commented Jun 16, 2020

No description provided.

@jhuan31
Copy link
Author

jhuan31 commented Jun 16, 2020

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

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.

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
Copy link
Author

jhuan31 commented Jun 17, 2020

@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
Copy link
Author

jhuan31 commented Jun 17, 2020

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
Copy link
Contributor

hanm commented Jun 18, 2020

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
Copy link
Contributor

hanm commented Jun 18, 2020

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
Copy link
Author

jhuan31 commented Jun 23, 2020

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