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

Fix regression of client offloadNone() strategy #2192

Merged
merged 2 commits into from
Jun 8, 2022

Conversation

bondolo
Copy link
Contributor

@bondolo bondolo commented Apr 15, 2022

Motivation:
The changes in #2108 introduced a regression in the handling of
computation of the effective strategy of a client when a
strategy offloadNone() (or the deprecated offloadNever()) was used
with the HttpClientBuilder. Rather than preventing all offloading,
the computed offloading was used.
Modifications:
If the strategy specified on the HttpClientBuilder does not specify
any offloads then no offloading will be used.
Result:
The offloading behavior matches the requested behavior specified on
the HttpClientBuilder.

@bondolo bondolo requested a review from idelpivnitskiy April 15, 2022 23:49
@bondolo bondolo self-assigned this Apr 15, 2022
@bondolo bondolo added the bug Something isn't working label Apr 15, 2022
@Scottmitch
Copy link
Member

@bondolo - test failure occurred, is it related to these changes?

@bondolo
Copy link
Contributor Author

bondolo commented Apr 18, 2022

The test's assumptions appear to be incorrect. I will take a look

bondolo added 2 commits June 7, 2022 15:08
Motivation:
The changes in apple#2108 introduced a regression in the handling of
computation of the effective strategy of a client when a
strategy `offloadNone()` (or the deprecated `offloadNever()`) was used
with the  `HttpClientBuilder`. Rather than preventing all offloading,
the computed offloading was used.
Modifications:
If the strategy specified on the `HttpClientBuilder` does not specify
any offloads then no offloading will be used.
Result:
The offloading behavior matches the requested behavior specified on
the  `HttpClientBuilder`.
@bondolo bondolo force-pushed the offloadNone-regression branch from f7cbfe2 to 5b59b43 Compare June 7, 2022 22:09
@bondolo
Copy link
Contributor Author

bondolo commented Jun 7, 2022

The fix to this regression was independently fixed in #2166. This PR extracts the only the necessary fix along with required test changes.

@bondolo bondolo removed the request for review from chemicL June 7, 2022 22:11
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.

Thank you!

@idelpivnitskiy idelpivnitskiy merged commit 6afe86b into apple:main Jun 8, 2022
@bondolo bondolo deleted the offloadNone-regression branch June 8, 2022 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants