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.

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

Fix FindCoordinatorResponse.encode to allow nil Coordinator #1050

Merged
merged 3 commits into from
Feb 14, 2018

Conversation

horkhe
Copy link
Contributor

@horkhe horkhe commented Feb 14, 2018

FindCoordinatorResponse.encode function panics if Coordinator is nil, so I fixed that. Besides I added MockFindCoordinatorResponse to use in tests.

return nil
}
pe.putInt32(0)
if f.Version >= 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not matched by the fact that we hard-code passing version 0 into Coordinator.encode?

Copy link

@buyology buyology Feb 14, 2018

Choose a reason for hiding this comment

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

Hm. We are currently sharing the same decode/encode method for Metadata* and FindCoordinator* but since version 1 of the MetadataResponse they are not the equal.

Metadata has rack whereas FindCoordinator does not — so actually 69-73 can be dropped

@eapache
Copy link
Contributor

eapache commented Feb 14, 2018

It would be nice if you could add a test for this case.

@buyology
Copy link

buyology commented Feb 14, 2018

Good catch. Maybe we should line up the test and encoding of a nil coordinator with the actual response where NO_NODE is defined like so:

    private static final Node NO_NODE = new Node(-1, "", -1);

@horkhe
Copy link
Contributor Author

horkhe commented Feb 14, 2018

@eapache @buyology done. Version is not properly used in encoding/decoding. NoNode was introduced and used in encoding if Coordinator is nil. The test covers both versions.

@eapache eapache merged commit f93325f into IBM:master Feb 14, 2018
@buyology
Copy link

buyology commented Feb 14, 2018

Sorry if I was unclear about my comment re: the shared decode/encode.

The short of it was that the FindCoordinatorRequest/Response does not have a rack-field:
https://kafka.apache.org/protocol#The_Messages_FindCoordinator

Sending a followup patch so let's continue there

@horkhe
Copy link
Contributor Author

horkhe commented Feb 15, 2018

Good catch, thank you @buyology.

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