[refactor] use block_waiter instead of batch_loader #738

akichidis · 2022-08-10T17:23:43Z

Resolves #706

This PR is:

deleting the batch_loader in executor
introducing the channels to use the block_waiter to fetch the blocks/payloads
introducing the temp_batch_store instead of reusing the batch_store where the later is meant to be used from the worker nodes. Also on the temp_batch_store we have the ability to store the unserialised Batch directly leading to simplification.
adding retry logic to fetch the blocks until successfully fetched (or permanently fail because of irrecoverable error)
adding end to end test to confirm the consensus output is successfully received
removing the code to replace the worker addresses part of the fix fix: Mitigate issue #706 #715

NOTE: Please review this first / as well #746

bmwill · 2022-08-10T18:08:05Z

executor/src/subscriber.rs

+                Ok(block) => {
+                    // we successfully received the payload. Now let's add to store
+                    store
+                        .write_all(block.batches.iter().map(|b| (b.id, b.transactions.clone())))


Suggested change

.write_all(block.batches.iter().map(|b| (b.id, b.transactions.clone())))

.write_all(block.batches.into_iter().map(|b| (b.id, b.transactions)))

akichidis · 2022-08-11T18:13:37Z

executor/src/subscriber.rs

@@ -1,95 +1,103 @@
 // Copyright (c) 2022, Mysten Labs, Inc.


Theoretically I could eliminate the Subscriber as well and embed the functionality in the executor::Core . However, the separation with the Subscriber offers us buffering capabilities, ie if the Core for some reason stuck we can still continue downloading the certificates' batches and output in the tx_executor channel, effectively saving time. Of course if tx_executor fills up we'll stall as well, but that's desired.

we may not need a separate thread for the subscriber but I would keep the logic in the core as simple as possible (so Subscriber may remain but perhaps not spawn its own tokio task?)

hhm what do you mean no spawn it's own tokio task? I mean, if we don't want to make subscriber block we need to run our loop in a task. Unless you mean something different?

We can either keep the subscriber as it is (in this PR) or make it a collection of functions that are called by the Executor Core

asonnino · 2022-08-11T20:15:35Z

executor/src/subscriber.rs

@@ -1,95 +1,103 @@
 // Copyright (c) 2022, Mysten Labs, Inc.


we may not need a separate thread for the subscriber but I would keep the logic in the core as simple as possible (so Subscriber may remain but perhaps not spawn its own tokio task?)

executor/src/subscriber.rs

asonnino · 2022-08-11T20:17:05Z

executor/src/subscriber.rs

+        // It's important to have the futures in ordered fashion as we want
+        // to guarantee that will deliver to the executor the certificates
+        // in the same order we received from rx_consensus. So it doesn't
+        // mater if we somehow managed to fetch the batches from a later
+        // certificate. Unless the earlier certificate's payload has been
+        // fetched, no later certificate will be delivered.


good comment

asonnino · 2022-08-11T20:18:25Z

executor/src/subscriber.rs

                tx_executor,
+                tx_get_block_commands,
+                get_block_retry_policy,
            }
            .run()
            .await
            .expect("Failed to run subscriber")


ha cool, if an unrecoverable error happens we shut down here.

Exactly, please also see the test here where I am explicitly checking that subscriber will crash in this case.

You can also see the recoverable / irrecoverable cases here . Errors that are mapped to Transient errors will make the retrier retry again. Errors that are marked as Permanent will make the retrier stop and eventually make the subscriber crash.

#707 should also help with the crashing :)

huitseeker

Excellent work, thanks!

huitseeker · 2022-08-14T17:49:07Z

primary/src/metrics.rs

@@ -69,7 +69,7 @@ pub struct PrimaryChannelMetrics {
    pub tx_primary_messages: IntGauge,
    /// occupancy of the channel from the `primary::PrimaryReceiverHandler` to the `primary::Helper`
    pub tx_helper_requests: IntGauge,
-    /// occupancy of the channel from the `primary::ConsensusAPIGrpc` to the `primary::BlockWaiter`
+    /// occupancy of the channel from the `primary::ConsensusAPIGrpc` & `executor::Subscriber` to the `primary::BlockWaiter`


I'd add one line saying that in contexts with an external consensus, it will be the former, and when the consensus is internal, it will be the later. This should make clear we don't actually expect 2 simultaneous senders yet.

This commit is swapping the batch_loader with the block_waiter in the executor. This allow us to use a common solution for fetching batches and take advantage of the existing payload sync capabilities. Also earlier fix code has been removed for swapping external worker addresses with internal ones.

This reverts commit babd3d8.

…bs#738)" This reverts commit babd3d8.

This reverts commit babd3d8.

…al#738) This commit is swapping the batch_loader with the block_waiter in the executor. This allow us to use a common solution for fetching batches and take advantage of the existing payload sync capabilities. Also earlier fix code has been removed for swapping external worker addresses with internal ones.

bmwill reviewed Aug 10, 2022

View reviewed changes

akichidis force-pushed the feat/use-block-waiter-in-executor branch from e182598 to 2cfb29b Compare August 11, 2022 17:43

akichidis mentioned this pull request Aug 11, 2022

[fix] block_waiter serve blocks with batches of similar digest ids #746

Merged

akichidis marked this pull request as ready for review August 11, 2022 18:09

akichidis requested a review from asonnino as a code owner August 11, 2022 18:09

akichidis requested review from huitseeker and arun-koshy August 11, 2022 18:09

akichidis commented Aug 11, 2022

View reviewed changes

asonnino approved these changes Aug 11, 2022

View reviewed changes

huitseeker approved these changes Aug 14, 2022

View reviewed changes

extract the tx_get_block_command channel to the node level

da5b9f4

akichidis force-pushed the feat/use-block-waiter-in-executor branch from 2cfb29b to da5b9f4 Compare August 15, 2022 16:19

akichidis added 2 commits August 15, 2022 17:39

fix workspace hack

bf44472

address review comments

3462973

akichidis enabled auto-merge (squash) August 15, 2022 17:21

akichidis merged commit babd3d8 into main Aug 15, 2022

akichidis deleted the feat/use-block-waiter-in-executor branch August 15, 2022 17:31

huitseeker mentioned this pull request Aug 15, 2022

chore(ci): retire ubuntu-latest for performance-sensitive jobs #780

Merged

huitseeker mentioned this pull request Aug 17, 2022

Committee channel being dropped and causing nodes to shutdown #805

Closed

This was referenced Oct 13, 2022

Failing benchmarks MystenLabs/sui#5212

Closed

Revert 818 + increase the batch timeout #859

Merged

[synchronizer] Allow different configurations for Narwhal block waiter timeouts in different setups MystenLabs/sui#5210

Open

huitseeker added a commit that referenced this pull request Sep 6, 2022

Revert "[refactor] use block_waiter instead of batch_loader (#738)"

81b72ee

This reverts commit babd3d8.

huitseeker added a commit that referenced this pull request Sep 6, 2022

Revert "[refactor] use block_waiter instead of batch_loader (#738)"

293cecd

This reverts commit babd3d8.

huitseeker mentioned this pull request Sep 6, 2022

Revert #738 & #842 #900

Closed

huitseeker added a commit to huitseeker/narwhal that referenced this pull request Sep 7, 2022

Revert "[refactor] use block_waiter instead of batch_loader (MystenLa…

d08ee3f

…bs#738)" This reverts commit babd3d8.

huitseeker added a commit that referenced this pull request Sep 7, 2022

Revert "[refactor] use block_waiter instead of batch_loader (#738)"

e44a82f

This reverts commit babd3d8.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] use block_waiter instead of batch_loader #738

[refactor] use block_waiter instead of batch_loader #738

akichidis commented Aug 10, 2022 •

edited

Loading

bmwill Aug 10, 2022

akichidis Aug 11, 2022

asonnino Aug 11, 2022

akichidis Aug 12, 2022

asonnino Aug 12, 2022

asonnino Aug 11, 2022

asonnino Aug 11, 2022

akichidis Aug 12, 2022

asonnino Aug 11, 2022

akichidis Aug 12, 2022

huitseeker Aug 14, 2022

huitseeker left a comment

huitseeker Aug 14, 2022

	.write_all(block.batches.iter().map(\|b\| (b.id, b.transactions.clone())))
	.write_all(block.batches.into_iter().map(\|b\| (b.id, b.transactions)))

[refactor] use block_waiter instead of batch_loader #738

[refactor] use block_waiter instead of batch_loader #738

Conversation

akichidis commented Aug 10, 2022 • edited Loading

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

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akichidis commented Aug 10, 2022 •

edited

Loading