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

KAFKA-7755 Turn update inet addresses #6049

Merged
merged 4 commits into from Jan 7, 2019

Conversation

@hackerwin7
Copy link
Contributor

commented Dec 19, 2018

KAFKA-7755

Turn update resolved addresses in case that node address maybe changed in DNS latter.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)
@hackerwin7 hackerwin7 changed the title KAFKA-7755 urn update inet addresses KAFKA-7755 Turn update inet addresses Dec 19, 2018
@@ -117,7 +117,7 @@ public void connecting(String id, long now, String host, ClientDnsLookup clientD
connectionState.moveToNextAddress();
} else {
nodeState.put(id, new NodeConnectionState(ConnectionState.CONNECTING, now,
this.reconnectBackoffInitMs, ClientUtils.resolve(host, clientDnsLookup)));
this.reconnectBackoffInitMs, ClientUtils.resolve(host, clientDnsLookup), clientDnsLookup));

This comment has been minimized.

Copy link
@loicmonney

loicmonney Dec 19, 2018

Couldn't we pass the host to the constructor here as well and do the DNS resolution with the real host instead of trying to have the canonical hostname? (remove getCanonicalHostName)

This comment has been minimized.

Copy link
@hackerwin7

hackerwin7 Dec 20, 2018

Author Contributor

Yeah, It dose make sense!

@hackerwin7

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2018

@loicmonney Could you please review again? Thanks

@hackerwin7

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

@ijuma Could you have time for this?

Copy link
Contributor

left a comment

@rajinisivaram would be a good person to review this.

@rajinisivaram

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

@hackerwin7 Could we add one unit test?

@hackerwin7

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

@rajinisivaram Thanks your reply!
I have no idea to how to change host mapping entry in DNS lookup, so I add some Visible for testing methods to simulate this scene.
Do you have any ideas to change or override single host mapping of DNS lookup in unit test?

}

/* Visible for testing */
protected void changeHostForNode(String nodeId, String host) {

This comment has been minimized.

Copy link
@edoardocomar

edoardocomar Jan 4, 2019

Contributor

visibility could be restricted to be package-level (i.e remove keyword protected)

This comment has been minimized.

Copy link
@rajinisivaram

rajinisivaram Jan 4, 2019

Contributor

Perhaps it would be better to change host using reflection in the test since this code looks out of place in the implementation? Then the host field can also be made final.

Copy link
Contributor

left a comment

@hackerwin7 Thanks for the update. Left a couple of minor comments.

@@ -25,6 +25,7 @@
import static org.junit.Assert.assertTrue;

import java.net.InetAddress;
import java.net.InetSocketAddress;

This comment has been minimized.

Copy link
@rajinisivaram

rajinisivaram Jan 4, 2019

Contributor

Unused import - this is causing checkstyle failure in the PR build.

}

/* Visible for testing */
protected void changeHostForNode(String nodeId, String host) {

This comment has been minimized.

Copy link
@rajinisivaram

rajinisivaram Jan 4, 2019

Contributor

Perhaps it would be better to change host using reflection in the test since this code looks out of place in the implementation? Then the host field can also be made final.

@hackerwin7

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

@rajinisivaram @edoardocomar Thanks for the great ideas!
Reflection is exactly a better choice for this.
Would you have time for this again? Thanks.

@rajinisivaram

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

retest this please

Copy link
Contributor

left a comment

@hackerwin7 Thanks for the updates, LGTM. Don't think test failures are related, but rerunning once anyway. Will merge after builds complete.

@rajinisivaram

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

Merging to trunk and 2.1

@rajinisivaram rajinisivaram merged commit e4f233e into apache:trunk Jan 7, 2019
1 of 2 checks passed
1 of 2 checks passed
JDK 8 and Scala 2.11 FAILURE 10252 tests run, 5 skipped, 1 failed.
Details
JDK 11 and Scala 2.12 SUCCESS 10252 tests run, 5 skipped, 0 failed.
Details
rajinisivaram added a commit that referenced this pull request Jan 7, 2019
…#6049)

Lookup client host name after every full iteration through the addresses returned.

Reviewers: Loïc Monney <loicmonney@github.com>, Edoardo Comar <ecomar@uk.ibm.com>, Rajini Sivaram <rajinisivaram@googlemail.com>
@loicmonney

This comment has been minimized.

Copy link

commented Jan 7, 2019

Hi @rajinisivaram, thank you for the review/merge! As this is quite an important issue for all projects running on top of Kubernetes (clients lose connection after a rolling restart) and knowing it is not possible to downgrade to previous versions, do you know when this fix will be part of any release?

@rajinisivaram

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

@loicmonney The fix will be in 2.1.1 which should be out soon (not sure of the date yet).

@hackerwin7

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Pengxiaolong added a commit to Pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…apache#6049)

Lookup client host name after every full iteration through the addresses returned.

Reviewers: Loïc Monney <loicmonney@github.com>, Edoardo Comar <ecomar@uk.ibm.com>, Rajini Sivaram <rajinisivaram@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.