This repository has been archived by the owner on Oct 17, 2022. It is now read-only.
Simplify executor-consensus interface #691
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
huitseeker
approved these changes
Aug 6, 2022
| @@ -185,29 +84,26 @@ impl Subscriber { | |||
| tokio::select! { | |||
| // Receive the ordered sequence of consensus messages from a consensus node. | |||
| Some(message) = self.rx_consensus.recv() => { | |||
| // Process the consensus message (synchronize missing messages, download transaction data). | |||
| let sequence = self.handle_consensus_message(&message).await?; | |||
There was a problem hiding this comment.
I am so happy to see this line go away.
LefKok
approved these changes
Aug 8, 2022
gdanezis
approved these changes
Aug 8, 2022
| waiting.push(future); | ||
| } | ||
| // Send the certificate to the batch loader to download all transactions' data. | ||
| self.tx_batch_loader |
There was a problem hiding this comment.
Is this a metered channel? Are we sure if this blocks it will not take down a tokio select that is required for liveness? Should we make it print a warn then error if the delay is too long? (I have become paranoid now).
There was a problem hiding this comment.
It is indeed not metered, but it seems that the last PR of @huitseeker fixes this. The batch loader can only block if the network blocks.
executor/src/subscriber.rs
Outdated
| .expect("Failed to send message ot batch loader"); | ||
|
|
||
| // Wait for the transaction data to be available in the store. We will then forward these | ||
| // transactions to the Executor Core for execution. |
There was a problem hiding this comment.
Maybe tell us a little more in this comment how this is guaranteed to happen, and therefore liveness if safe?
| .await | ||
| .map_err(|_| SubscriberError::ExecutorConnectionDropped)?, | ||
| Some(message) = waiting.next() => { | ||
| if self.tx_executor.send(message?).await.is_err() { |
There was a problem hiding this comment.
Same questions about sending as above.
akichidis
approved these changes
Aug 8, 2022
| @@ -300,16 +284,14 @@ impl Node { | |||
| store.batch_store.clone(), | |||
| execution_state, | |||
| tx_reconfigure, | |||
| /* rx_consensus */ rx_consensus_to_client, | |||
| /* tx_consensus */ tx_client_to_consensus, | |||
huitseeker
pushed a commit
to huitseeker/narwhal
that referenced
this pull request
Aug 8, 2022
Remove useless sync
Merged
3 tasks
huitseeker
added a commit
to MystenLabs/sui
that referenced
this pull request
Aug 9, 2022
mwtian
pushed a commit
to mwtian/sui
that referenced
this pull request
Sep 30, 2022
Remove useless sync
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simplify the consensus-executor interface by removing a sync that is not used. This sync was originally added when we planned to run the executor (and Sui) on a different machine than the consensus