Skip to content

KAFKA-8643: bring back public MemberDescription constructor#7060

Merged
hachikuji merged 5 commits intoapache:trunkfrom
abbccdda:member_description
Jul 12, 2019
Merged

KAFKA-8643: bring back public MemberDescription constructor#7060
hachikuji merged 5 commits intoapache:trunkfrom
abbccdda:member_description

Conversation

@abbccdda
Copy link
Copy Markdown

@abbccdda abbccdda commented Jul 9, 2019

a compatibility fix aiming to avoid breaking user's code by accidentally removing a public constructor.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@abbccdda abbccdda force-pushed the member_description branch from d8e5e85 to 54d3088 Compare July 9, 2019 23:48
@abbccdda
Copy link
Copy Markdown
Author

@ijuma @hachikuji FYI

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jul 10, 2019

Thanks for the PR. The comments seem a bit weird, binary compatibility is a requirement when changing classes that are part of the public API. Adding the comments to an arbitrary subset adds confusion in my opinion.

Also, did the KIP state that the constructor would be deprecated?

@abbccdda
Copy link
Copy Markdown
Author

@ijuma I agree that we don't need extra comment. And the KIP didn't state that we want to deprecate the constructor. Just add back the constructor to fix the problem only.

@abbccdda
Copy link
Copy Markdown
Author

@hachikuji Thanks for the catch. Addressed!

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jul 10, 2019

Can we please add a test that verifies the equals/hashCode change?

@abbccdda abbccdda force-pushed the member_description branch from 6ffd125 to ca72235 Compare July 10, 2019 17:35
@abbccdda abbccdda force-pushed the member_description branch from ca72235 to 06ea20c Compare July 10, 2019 17:48
@abbccdda
Copy link
Copy Markdown
Author

@ijuma Thanks for the suggestion, added

@abbccdda abbccdda closed this Jul 11, 2019
@abbccdda abbccdda reopened this Jul 11, 2019
@abbccdda
Copy link
Copy Markdown
Author

Triggering a build through reopening.

@hachikuji
Copy link
Copy Markdown
Contributor

retest this please

@abbccdda abbccdda force-pushed the member_description branch from fb8de8f to 41f97f0 Compare July 11, 2019 17:26
@abbccdda
Copy link
Copy Markdown
Author

The only failure is org.apache.kafka.connect.integration.ExampleConnectIntegrationTest > testSourceConnector FAILED

Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the patch!

@hachikuji hachikuji merged commit 25f4e3c into apache:trunk Jul 12, 2019
bfncs pushed a commit to bfncs/kafka that referenced this pull request Jul 12, 2019
)

This patch fixes a compatibility breaking `MemberDescription` constructor change in apache#6957. It also updates `equals` and `hashCode` for the new `groupInstanceId` field that was added in the same patch.

Reviewers: Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>
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