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

MultiAddressClientBuilder execution strategy control #2166

Merged
merged 64 commits into from
Jun 15, 2022

Conversation

bondolo
Copy link
Contributor

@bondolo bondolo commented Mar 29, 2022

Motivation:
The MultiAddressClientBuilder does not currently always respect
overrides of execution strategy made by the single address
initializer
Modification:
The computed execution strategy is a combination of the multiaddress
client builder strategy, the api strategy and the client strategy.
Result:
More predictable and controllable behavior for MultiAddressClientBuilder

@bondolo bondolo added the bug Something isn't working label Mar 29, 2022
@bondolo bondolo self-assigned this Mar 29, 2022
Motivation:
The `MultiAddressClientBuilder` does not currently always respect
overrides of execution strategy made by the single address
initializer
Modification:
The computed execution strategy is a combination of the multiaddress
client builder strategy, the api strategy and the client strategy.
Result:
More predictable and controllable behavior for `MultiAddressClientBuilder`
@bondolo bondolo force-pushed the multiaddress-initializer-strategy branch from aabc697 to dc260f4 Compare March 29, 2022 21:54
@bondolo bondolo marked this pull request as ready for review April 5, 2022 01:26
@bondolo bondolo requested review from idelpivnitskiy, chemicL, Scottmitch and tkountis and removed request for chemicL April 5, 2022 01:26
@bondolo bondolo requested a review from idelpivnitskiy April 14, 2022 23:23
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.

LGTM after comments addressed:

@bondolo bondolo force-pushed the multiaddress-initializer-strategy branch from 03198b9 to acd2535 Compare June 6, 2022 23:18
@idelpivnitskiy
Copy link
Member

Will have a final look after merge conflicts resolved

@bondolo bondolo marked this pull request as draft June 8, 2022 18:23
@bondolo
Copy link
Contributor Author

bondolo commented Jun 8, 2022

Temporarily draft as pieces are pulled into separate standalone PRs.

@bondolo bondolo marked this pull request as ready for review June 10, 2022 18:22
@bondolo
Copy link
Contributor Author

bondolo commented Jun 10, 2022

@bondolo bondolo requested a review from idelpivnitskiy June 10, 2022 19:02
@bondolo bondolo dismissed idelpivnitskiy’s stale review June 14, 2022 21:02

review feedback all implemented.

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.

🚀

@idelpivnitskiy
Copy link
Member

Hey, some outdated comments from intermediate iterations were published unintentionally. All of those are already resolved. Go ahead with merging the PR. Thanks for the hard work on it!

@bondolo bondolo merged commit 0f1ac15 into apple:main Jun 15, 2022
@bondolo bondolo deleted the multiaddress-initializer-strategy branch June 15, 2022 20:01
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.

2 participants