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

[WIP] Bypass http call if the node is local #13838

Closed
wants to merge 1 commit into from

Conversation

dharakk
Copy link
Contributor

@dharakk dharakk commented Dec 10, 2019

#13937
If a global config is set and the node-uri matches the current
node, then use the task manager directly for fetching exchange data and
doing task calls. That is bypass the taskResource if these conditions
are satisfied.

In addition, the backoff changed such that the local mode
never undergoes a backoff. It's a bit hacky but does the job.

Can be enabled (default is disabled) by setting
task.bypass-http-for-local=true

In the low latency one-node setup (designed by @agrawaldevesh ), large sum of latency stemmed from http calls on the local host, this change helps shave of that latency piece

== RELEASE NOTES ==

General Changes
* Bypass http call if the node is local with global config

If a global config is set and the node-uri matches the current
node, then use the task manager directly for fetching exchange data and
doing task calls. That is bypass the taskResource if these conditions
are satisfied.

In addition, the backoff changed  such that the local mode
never undergoes a backoff. It's a bit hacky but does the job.

Can be enabled (default is disabled) by setting
task.bypass-http-for-local=true
@dharakk dharakk marked this pull request as ready for review December 20, 2019 23:09
@mbasmanova mbasmanova self-assigned this Jan 7, 2020
@highker highker requested a review from wenleix January 7, 2020 01:49
@mbasmanova mbasmanova assigned highker and unassigned mbasmanova Jan 7, 2020
@highker
Copy link
Contributor

highker commented Jan 7, 2020

@dharakk, I just briefly skimmed through the PR and here are some high-level comments:

  • having a communication tunnel bypassing network is ok to proceed
  • we need better abstraction so that the local communication has nothing to do with HttpClient. Support internal communication with thrift #13894 shows an abstraction of how we can leverage it
  • Given Support internal communication with thrift #13894 is there, could you rebase on top of it or wait till it's get reviewed (cc: @wenleix)? Because that patch has already provided you the abstraction. Be aware that the high-level principal of how exchange sink/source should be designed should be orthogonal from the detailed implementation (http, http2, thrift, grpc, or local function calls). A good abstraction is needed to achieve that.

@highker highker assigned dharakk and unassigned highker Jan 7, 2020
@dharakk
Copy link
Contributor Author

dharakk commented Jan 7, 2020

@dharakk, I just briefly skimmed through the PR and here are some high-level comments:

  • having a communication tunnel bypassing network is ok to proceed
  • we need better abstraction so that the local communication has nothing to do with HttpClient. Support internal communication with thrift #13894 shows an abstraction of how we can leverage it
  • Given Support internal communication with thrift #13894 is there, could you rebase on top of it or wait till it's get reviewed (cc: @wenleix)? Because that patch has already provided you the abstraction. Be aware that the high-level principal of how exchange sink/source should be designed should be orthogonal from the detailed implementation (http, http2, thrift, grpc, or local function calls). A good abstraction is needed to achieve that.

Thanks @highker for the review. Having an abstraction is definitely the cleaner approach. I will rebase on top of #13894 and update

Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

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

As discussed, let's rebase this on top of #13894 with proper abstraction.

@stale
Copy link

stale bot commented Jul 6, 2020

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

@stale stale bot added the stale label Jul 6, 2020
@stale stale bot closed this Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants