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

Reduce dns lookup overhead on NodeScheduler #18365

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

guhanjie
Copy link
Member

@guhanjie guhanjie commented Jul 21, 2023

Description

Reduce dns lookup overhead on NodeScheduler

Additional context and related issues

It does not need to dns lookup for hostname if HostAddress has port, which incurred additional overhead when dns too slow on some host environment.

Release notes

(x) This is not user-visible or docs only and no release notes are required. In case someone things differently, here is a stub:

# release notes stub (but perhaps no Rn needed)

* Reduce dns lookup overhead on NodeScheduler.
   This can avoid unnecessary dns lookup for HostAddress has port, while previous would incurred additional overhead when dns too slow on some host environment.

It does not need to dns lookup for hostname if HostAddress has port, which incurred additional overhead when dns too slow on some host environment.
@cla-bot cla-bot bot added the cla-signed label Jul 21, 2023
@guhanjie guhanjie self-assigned this Jul 21, 2023
@guhanjie guhanjie requested a review from findepi July 21, 2023 06:14
@guhanjie
Copy link
Member Author

@findepi I had one ci check(test-jdbc-compatibility) not passed, I want to re-run it, but can't find the trigger entry. Could you help me, thanks

@findepi
Copy link
Member

findepi commented Jul 21, 2023

Does this matter only when multiple worker nodes on single host?
If so, I don't think this is anyhow recommended scenario.
Am i reading this wrong?

@guhanjie
Copy link
Member Author

guhanjie commented Jul 21, 2023

Does this matter only when multiple worker nodes on single host? If so, I don't think this is anyhow recommended scenario. Am i reading this wrong?

@findepi It will must do host.toInetAddress(); whenever one/multi workers on host, but that is unnecessary when nodes has port.
And it would be too slow or hang if dns lookup has problem on some environment as possible as we have.

So, I think the block code before if (!host.hasPort()) { is absolutely unnecessary when host has port.
It should be optimized as pull request.

@guhanjie
Copy link
Member Author

guhanjie commented Aug 4, 2023

Does this matter only when multiple worker nodes on single host? If so, I don't think this is anyhow recommended scenario. Am i reading this wrong?

@findepi I suppose you are reading wrong, please check it again.

@guhanjie
Copy link
Member Author

guhanjie commented Aug 4, 2023

@findepi Does this commit have any problem?

@guhanjie guhanjie requested review from martint and findepi and removed request for findepi August 6, 2023 11:19
@guhanjie
Copy link
Member Author

guhanjie commented Aug 6, 2023

@martint Could you have a review for this? It seems @findepi always missing my message...

@findepi findepi merged commit 4cacdb8 into trinodb:master Aug 7, 2023
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Aug 7, 2023
@github-actions github-actions bot added this to the 423 milestone Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

2 participants