Support quorum callbacks for AsyncResult#12957
Conversation
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
gxglass
left a comment
There was a problem hiding this comment.
AI doesn't find issues, and the pieces make sense to me to the extent I can understand them in a number of hours of reading.
Additional comments on scope and function of some of the more important objects may benefit future readers.
Thanks for adding the requested test cases. AI has a few minor enhancements suggested (cases not already covered) which I think are not necessary.
I think the biggest suggestion I can offer is, is there a way to test this at scale with some amount of isolation? Like do a sort of meta simulation that runs for say 30s and creates a lot of futures/async results, quorums of various sizes, with a reasonable mix of success/failure/cancellation, and just let the machinery run flat out for 30s. At the end make some reasonable assertions that memory hasn't been leaked (e.g. RSS is within some tolerable amount of what we started at). This also gives *SAN checkers something to chew on to look for data races, lifetime issues, etc. I think having this separate from vanilla simulation would make it easier to debug subtle issues in here if any without having to wade through possibly unpredictable full-blown simulation errors.
|
|
||
| template <class T> | ||
| struct Quorum final : SAV<Void> { | ||
| // Shared SAV-backed quorum bookkeeping used by both Future and AsyncResult |
There was a problem hiding this comment.
Does the GetAllAsyncResult final : SAV<std::vector> below render this comment out of date?
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Now that this PR is merged, I think we can get this test coverage once the first real use case is implemented, migrating |
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|

This PR extends
quorumto support astd::vector<AsyncResult<T>>overload. This extends the cases in whichAsyncResultcan be used to avoid unnecessary copies. For example, the following pattern is possible:This PR is necessary to convert
Status.actor.cppto standard coroutines without introducing a performance regression.Code-Reviewer Section
The general pull request guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branchormainif this is the youngest branch)