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.

Sign up for GitHub

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

Jump to bottom

Schedule/execute no offload timeouts on IoExecutor #2070

Merged
merged 2 commits into from
Feb 2, 2022
Merged

Schedule/execute no offload timeouts on IoExecutor #2070

merged 2 commits into from
Feb 2, 2022

Conversation

bondolo
Copy link
Contributor

@bondolo bondolo commented Feb 1, 2022 •
edited
Loading

Motivation:
Currently the timeout filter executes timeout events on the execution
context executor even if the execution strategy requires no offloading.
The IoExecutor should instead be use for these situations to reduce
offloads.
Modifications:
Request ExecutionContext.ioExecutor() as the contextExecutor if no
offloads are configured.
Result:
Less offloading for some timeout events.

@bondolo bondolo requested review from idelpivnitskiy, chemicL and tkountis and removed request for chemicL February 1, 2022 01:45
@bondolo bondolo self-assigned this Feb 1, 2022
@bondolo bondolo added the enhancement New feature or request label Feb 1, 2022
@bondolo bondolo requested review from chemicL and Scottmitch February 1, 2022 01:45
@bondolo bondolo marked this pull request as draft February 1, 2022 19:17
@bondolo
Copy link
Contributor Author

bondolo commented Feb 1, 2022

This will now depend, again, upon #2066. I will move it out of draft when that PR is complete.

@bondolo bondolo marked this pull request as ready for review February 2, 2022 00:43
@bondolo bondolo requested a review from tkountis February 2, 2022 00:43
@bondolo bondolo changed the title Execute no offload timeouts on immediate Executor Schedule/execute no offload timeouts on IoExecutor Feb 2, 2022
idelpivnitskiy
idelpivnitskiy approved these changes Feb 2, 2022
View reviewed changes
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

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

🚀

...ncurrent-api/src/test/java/io/servicetalk/concurrent/api/publisher/TimeoutPublisherTest.java Show resolved Hide resolved
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/TimeoutHttpRequesterFilter.java Outdated Show resolved Hide resolved
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/TimeoutHttpServiceFilter.java Outdated Show resolved Hide resolved
...cetalk-http-utils/src/test/java/io/servicetalk/http/utils/AbstractTimeoutHttpFilterTest.java Outdated Show resolved Hide resolved
@bondolo
Schedule/execute no offload timeouts on IoExecutor
368ba76 
Motivation:
Currently the timeout filter executes timeout events on the execution
context executor even if the execution strategy requires no offloading.
The IoExecutor should instead be use for these situations to reduce
offloads.
Modifications:
Request `ExecutionContext.ioExecutor()` as the `contextExecutor` if no
offloads are configured.
Result:
Less offloading for some timeout events.
@bondolo
review feedback
a0ae767
@bondolo bondolo merged commit 5ecd4aa into apple:main Feb 2, 2022
@idelpivnitskiy
Copy link
Member

@bondolo do you plan to cherry-pick this change into 0.42 and 0.41?

@bondolo
Copy link
Contributor Author

bondolo commented Feb 4, 2022 •
edited
Loading

Yes, I will cherry pick to 0.42 but will have to check for 0.41 to make sure it can apply cleanly without too much work

bondolo added a commit that referenced this pull request Feb 4, 2022
@bondolo
Schedule/execute no offload timeouts on IoExecutor (#2070)
ae7dd0f 
Motivation:
Currently the timeout filter executes timeout events on the execution
context executor even if the execution strategy requires no offloading.
The IoExecutor should instead be use for these situations to reduce
offloads.
Modifications:
Request `ExecutionContext.ioExecutor()` as the `contextExecutor` if no
offloads are configured.
Result:
Less offloading for some timeout events.
bondolo added a commit to bondolo/servicetalk that referenced this pull request Feb 4, 2022
@bondolo
Schedule/execute no offload timeouts on IoExecutor (apple#2070)
461b70c 
Motivation:
Currently the timeout filter executes timeout events on the execution
context executor even if the execution strategy requires no offloading.
The IoExecutor should instead be use for these situations to reduce
offloads.
Modifications:
Request `ExecutionContext.ioExecutor()` as the `contextExecutor` if no
offloads are configured.
Result:
Less offloading for some timeout events.
@bondolo
Copy link
Contributor Author

bondolo commented Feb 4, 2022 •
edited
Loading

0.42 → ae7dd0f
0.41 → 580a5a2

bondolo added a commit that referenced this pull request Feb 4, 2022
@bondolo
Schedule/execute no offload timeouts on IoExecutor (#2070)
580a5a2 
Motivation:
Currently the timeout filter executes timeout events on the execution
context executor even if the execution strategy requires no offloading.
The IoExecutor should instead be use for these situations to reduce
offloads.
Modifications:
Request `ExecutionContext.ioExecutor()` as the `contextExecutor` if no
offloads are configured.
Result:
Less offloading for some timeout events.
bondolo added a commit to bondolo/servicetalk that referenced this pull request Feb 9, 2022
@bondolo
Revert API change involving Executor
f99e131 
Motivation:
A source compatible API change was made in apple#2070 that replaced
`io.servicetalk.concurrent.api.Executor` with
`io.servicetalk.concurrentExecutor`. This change, while source
compatible as `io.servicetalk.concurrentExecutor` is the super-type of
`io.servicetalk.concurrent.api.Executor`, is not binary compatible.
Modifications:
Revert change to use same `Executor` class as prior releases on these
branches.
Result:
Binary compatibility is retained.
@bondolo bondolo mentioned this pull request Feb 9, 2022
Merged
bondolo added a commit that referenced this pull request Feb 9, 2022
@bondolo
Revert API change involving Executor (#2087)
52d36f6 
Motivation:
A source compatible API change was made in #2070 that replaced
`io.servicetalk.concurrent.api.Executor` with
`io.servicetalk.concurrentExecutor`. This change, while source
compatible as `io.servicetalk.concurrentExecutor` is the super-type of
`io.servicetalk.concurrent.api.Executor`, is not binary compatible.
Modifications:
Revert change to use same `Executor` class as prior releases on these
branches.
Result:
Binary compatibility is retained.
idelpivnitskiy pushed a commit that referenced this pull request Feb 15, 2022
@bondolo @idelpivnitskiy
Revert API change involving Executor (#2087)
5625d27 
Motivation:
A source compatible API change was made in #2070 that replaced
`io.servicetalk.concurrent.api.Executor` with
`io.servicetalk.concurrentExecutor`. This change, while source
compatible as `io.servicetalk.concurrentExecutor` is the super-type of
`io.servicetalk.concurrent.api.Executor`, is not binary compatible.
Modifications:
Revert change to use same `Executor` class as prior releases on these
branches.
Result:
Binary compatibility is retained.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idelpivnitskiy idelpivnitskiy idelpivnitskiy approved these changes

@chemicL chemicL Awaiting requested review from chemicL

@Scottmitch Scottmitch Awaiting requested review from Scottmitch

@tkountis tkountis Awaiting requested review from tkountis

Assignees

@bondolo bondolo

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bondolo @idelpivnitskiy @tkountis