[decoupled-execution] process_sync_req now tries to advance a buffer item to aggregated #9207

yuxiamit · 2021-09-17T01:23:46Z

process_sync_request now tries to advance a buffer item to aggregated

Pop a prefix of buffer items if found an aggregated item.

Motivation

(Write your motivation for proposed changes here.)

Have you read the Contributing Guidelines on pull requests?

(Write your answer here.)

Test Plan

(Share your test plan here. If you changed code, please provide us with clear instructions for verifying that your changes work.)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/diem/diem/tree/main/developers.diem.com, and link to your PR here.)

If targeting a release branch, please fill the below out as well

Justification and breaking nature (who does it affect? validators, full nodes, tooling, operators, AOS, etc.)
Comprehensive test results that demonstrate the fix working and not breaking existing workflows.
Why we must have it for V1 launch.
What workarounds and alternative we have if we do not push the PR.

zekun000 · 2021-09-17T03:01:36Z

consensus/src/experimental/buffer_item.rs

+                if signed_item
+                    .commit_proof
+                    .commit_info()
+                    .match_ordered_only(ledger_info.commit_info())


this should be full match?

good point.

zekun000 · 2021-09-17T03:01:45Z

consensus/src/experimental/buffer_item.rs

+                let executed_item = *executed_item_box;
+                if executed_item
+                    .commit_info
+                    .match_ordered_only(ledger_info.commit_info())


zekun000 · 2021-09-17T03:04:59Z

consensus/src/experimental/buffer_manager.rs

+                    .map(|eb| Arc::new(eb.clone()))
+                    .collect::<Vec<Arc<ExecutedBlock>>>(),
+            );
+            if link_eq(&cursor, &aggregated_item_cursor) {


I'd prefer we break out the loop first then send the request, seems more readable

in this function I want to easily distinguish successfully finding the item vs reaching the end of the list, so I let it send the request here.

hmm, it's not possible to reach the end of the list without hitting the cursor right?

Right now it is not possible. Just in case of a bug in the future.

it seems more natural to have something like

while curson.is_some() { if find { break } } assert!(!blocks_to_persist.is_empty()); send_request

blocks_to_persist collects the blocks before (including) the aggregated item. If it does not find the item, blocks_to_persist contains all the blocks. I added a flag found and moved the code outside the loop.

oh, can't we just assert cursor.is_some() after the loop instead?

We have to move out buffer_item anyway, so I just put it into an option and check if buffer_item has something. The code is already updated.

I still feel we can simplify this code, but I guess I can try it myself after it gets merged

zekun000 · 2021-09-17T03:05:27Z

consensus/src/experimental/buffer_manager.rs

        }
+        unreachable!("Bad Aggregated Item Cursor: Item not found in the buffer.");


why this is unreachable?

This function assumes the aggregated item exists in the buffer.

it only break after sending the request instead of return

oops sorry I meant to return after sending the request

zekun000 · 2021-09-17T03:08:01Z

consensus/src/experimental/buffer_manager.rs

+                    let (attempted_item, aggregated, matching) =
+                        buffer_item.try_advance_to_aggregated_with_ledger_info(ledger_info.clone());
+                    set_elem(&cursor, attempted_item);
+                    if matching {


if aggregated is enough?

there are three cases:

not matched, not aggregated, we continue to the next iteration

matched, not aggregated, we end the loop since we already found the item

matched and aggregated, we call try_persisting to dequeue the items.

I mean aggregated must be matched?

yeah, but they share a break statement. So I put it inside matching.

zekun000 · 2021-09-17T19:01:12Z

consensus/src/experimental/buffer_manager.rs

+            self.buffer.pop_front();
+        }
+
+        if let Some(buffer_item) = found_item {


we can use expect here to avoid the if branch

sounds good. I'll do this in the next PR.

zekun000 · 2021-09-17T19:03:50Z

consensus/src/experimental/buffer_manager.rs

+                    .map(|eb| Arc::new(eb.clone()))
+                    .collect::<Vec<Arc<ExecutedBlock>>>(),
+            );
+            if link_eq(&cursor, &aggregated_item_cursor) {


I still feel we can simplify this code, but I guess I can try it myself after it gets merged

yuxiamit · 2021-09-17T19:29:04Z

/land

…item to aggregated. Pop a prefix of buffer items if found an aggregated item. Closes: #9207

bors-libra · 2021-09-17T20:51:07Z

💔 Test Failed - ci-test

yuxiamit · 2021-09-18T01:08:52Z

/land

…item to aggregated. Pop a prefix of buffer items if found an aggregated item. Closes: diem#9207

github-actions · 2021-09-18T01:51:38Z

Cluster Test Result

Test runner setup time spent 257 secs
Compatibility test results for land_6122cb78 ==> land_3e334126 (PR)
1. All instances running land_6122cb78, generating some traffic on network
2. First full node land_6122cb78 ==> land_3e334126, to validate new full node to old validator node traffic
3. First Validator node land_6122cb78 ==> land_3e334126, to validate storage compatibility
4. First batch validators (14) land_6122cb78 ==> land_3e334126, to test consensus and traffic between old full nodes and new validator node
5. First batch full nodes (14) land_6122cb78 ==> land_3e334126
6. Second batch validators (15) land_6122cb78 ==> land_3e334126, to upgrade rest of the validators
7. Second batch of full nodes (15) land_6122cb78 ==> land_3e334126, to finish the network upgrade, time spent 700 secs
all up : 1153 TPS, 3929 ms latency, 4500 ms p99 latency, no expired txns, time spent 249 secs
Logs: http://kibana.ct-0-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2021-09-18T01:28:41Z',to:'2021-09-18T01:51:37Z'))
Dashboard: http://grafana.ct-0-k8s-testnet.aws.hlw3truzy4ls.com/d/performance/performance?from=1631928521000&to=1631929897000
Validator 1 logs: http://kibana.ct-0-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2021-09-18T01:28:41Z',to:'2021-09-18T01:51:37Z'))&_a=(columns:!(log),query:(language:kuery,query:'kubernetes.pod_name:"val-1"'),sort:!(!('@timestamp',desc)))

Repro cmd:

./scripts/cti --tag land_6122cb78 --cluster-test-tag land_3e334126 -E BATCH_SIZE=15 -E UPDATE_TO_TAG=land_3e334126 --report report.json --suite land_blocking_compat

🎉 Land-blocking cluster test passed! 👌

yuxiamit requested a review from a team as a code owner September 17, 2021 01:23

bors-libra added this to In Review in bors Sep 17, 2021

diem-cla-bot bot added the cla-signed label Sep 17, 2021

yuxiamit requested a review from zekun000 September 17, 2021 01:23

zekun000 reviewed Sep 17, 2021

View reviewed changes

yuxiamit force-pushed the buffer-manager-sync-req-fix branch 4 times, most recently from ad8d221 to 4b7a885 Compare September 17, 2021 13:57

zekun000 reviewed Sep 17, 2021

View reviewed changes

zekun000 previously approved these changes Sep 17, 2021

View reviewed changes

bors-libra moved this from In Review to Queued in bors Sep 17, 2021

bors-libra pushed a commit that referenced this pull request Sep 17, 2021

[decoupled-execution] process_sync_req now tries to advance a buffer …

17d0fd0

…item to aggregated. Pop a prefix of buffer items if found an aggregated item. Closes: #9207

bors-libra moved this from Queued to Testing in bors Sep 17, 2021

bors-libra moved this from Testing to In Review in bors Sep 17, 2021

yuxiamit dismissed zekun000’s stale review via b7f44de September 18, 2021 01:07

yuxiamit force-pushed the buffer-manager-sync-req-fix branch from 4b7a885 to b7f44de Compare September 18, 2021 01:07

zekun000 previously approved these changes Sep 18, 2021

View reviewed changes

bors-libra moved this from In Review to Queued in bors Sep 18, 2021

[decoupled-execution] process_sync_req now tries to advance a buffer …

3e33412

…item to aggregated. Pop a prefix of buffer items if found an aggregated item. Closes: diem#9207

bors-libra moved this from Queued to Testing in bors Sep 18, 2021

bors-libra dismissed zekun000’s stale review via 3e33412 September 18, 2021 01:51

bors-libra force-pushed the buffer-manager-sync-req-fix branch from b7f44de to 3e33412 Compare September 18, 2021 01:51

bors-libra removed this from Testing in bors Sep 18, 2021

bors-libra closed this in 3e33412 Sep 18, 2021

bors-libra merged commit 3e33412 into diem:main Sep 18, 2021

bors-libra temporarily deployed to Sccache September 18, 2021 01:52 Inactive

bors-libra temporarily deployed to Docker September 18, 2021 01:52 Inactive

bors-libra temporarily deployed to Sccache September 18, 2021 14:18 Inactive

bors-libra temporarily deployed to Docker September 18, 2021 14:18 Inactive

bors-libra temporarily deployed to Sccache September 19, 2021 14:18 Inactive

bors-libra temporarily deployed to Docker September 19, 2021 14:18 Inactive

bors-libra temporarily deployed to Sccache September 20, 2021 14:19 Inactive

bors-libra temporarily deployed to Docker September 20, 2021 14:19 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[decoupled-execution] process_sync_req now tries to advance a buffer item to aggregated #9207

[decoupled-execution] process_sync_req now tries to advance a buffer item to aggregated #9207

yuxiamit commented Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit Sep 17, 2021

zekun000 Sep 17, 2021

yuxiamit commented Sep 17, 2021

bors-libra commented Sep 17, 2021

yuxiamit commented Sep 18, 2021

github-actions bot commented Sep 18, 2021

		}
		unreachable!("Bad Aggregated Item Cursor: Item not found in the buffer.");

[decoupled-execution] process_sync_req now tries to advance a buffer item to aggregated #9207

[decoupled-execution] process_sync_req now tries to advance a buffer item to aggregated #9207

Conversation

yuxiamit commented Sep 17, 2021

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

If targeting a release branch, please fill the below out as well

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuxiamit commented Sep 17, 2021

bors-libra commented Sep 17, 2021

yuxiamit commented Sep 18, 2021

github-actions bot commented Sep 18, 2021