Skip to content
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

[Zen2] Fix CoordinatorTests #34002

Merged
merged 8 commits into from
Sep 25, 2018
Merged

[Zen2] Fix CoordinatorTests #34002

merged 8 commits into from
Sep 25, 2018

Conversation

DaveCTurner
Copy link
Contributor

Today the CoordinatorTests are not very reliable if two elections are scheduled concurrently. Although we expect occasional failures due to this, in fact the failures are much more common than expected due to a handful of issues. This PR fixes these issues.

@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests v7.0.0 :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Sep 24, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

ywelsch
ywelsch reviewed Sep 24, 2018
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

I've left one comment, the rest LGTM

server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java Outdated
@@ -122,6 +122,10 @@ private void handleApplyCommit(ApplyCommitRequest applyCommitRequest) {
}

PublishWithJoinResponse handlePublishRequest(PublishRequest publishRequest) {
// overwrite local node
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is needed. As we're using the proper transport layer in our tests, this should be taken care of by PublicationTransportHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted, I hadn't realised that the transport layer should have been doing this when deserialising the request. The issue is that DisruptibleMockTransport doesn't de/serialize the requests before passing them on, so I've added that in defcc57.

@DaveCTurner
Copy link
Contributor Author

Ok. Unfortunately it makes the most sense to do this in MockTransport (since DisruptibleMockTransport doesn't have access to the response handler it needs to create the response object). There was similar behaviour in handleRemoteError() which rarely returns the original object so I copied that.

@DaveCTurner
Copy link
Contributor Author

Changed my mind, I think we should always de/serialize it.

@ywelsch ywelsch mentioned this pull request Sep 25, 2018
Closed
61 tasks
ywelsch
ywelsch approved these changes Sep 25, 2018
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

Great!

@DaveCTurner DaveCTurner merged commit 1d47c95 into elastic:zen2 Sep 25, 2018
@DaveCTurner DaveCTurner deleted the 2018-09-24-misc-fixes branch September 25, 2018 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywelsch ywelsch ywelsch approved these changes

Assignees
No one assigned
Labels
:Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >test Issues or PRs that are addressing/adding tests v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DaveCTurner @elasticmachine @ywelsch @colings86