-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_batcher: perform commitment tasks #11442
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
base: main-v0.14.1-committer
Are you sure you want to change the base?
apollo_batcher: perform commitment tasks #11442
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
fd812d6 to
2e05557
Compare
ac457db to
839ce6c
Compare
839ce6c to
144ffc1
Compare
4c0792e to
be8f623
Compare
144ffc1 to
a75f4b6
Compare
be8f623 to
4f4475a
Compare
a75f4b6 to
6a9a2d4
Compare
4f4475a to
b4d8a3a
Compare
6a9a2d4 to
2fd00f8
Compare
amosStarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amosStarkware reviewed 1 file and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yoavGrs).
crates/apollo_batcher/src/commitment_manager/state_committer.rs line 49 at r2 (raw file):
impl StateCommitter { /// Repeatedly performs any task in the channel.
Suggestion:
/// Performs the tasks in the channel. Retries at recoverable errors.crates/apollo_batcher/src/commitment_manager/state_committer.rs line 50 at r2 (raw file):
impl StateCommitter { /// Repeatedly performs any task in the channel. pub(crate) async fn perform_commitment_tasks(
the original name was before we decided to add revert tasks. please also rename perform_commitment_task.
you can also add a TODO on me if it's a lot of work
Suggestion:
perform_taskscrates/apollo_batcher/src/commitment_manager/state_committer.rs line 58 at r2 (raw file):
let output = perform_commitment_task(request, &committer_client).await; // TODO(Yoav): wait for task channel by config. results_sender.send(output).await.unwrap();
I don't think we need a config here:
the tasks channel may grow very large if the committer is lagging behind the batcher.
but the results channel shouldn't grow very large. I'll keep this discussion open in case Dori thinks otherwise.
please change to try_send, match on errors (for more descriptive panic messages), and log when a result is added. similar to this
Suggestion:
// TODO(Yoav): wait for task channel by config.
results_sender.try_send(output).await.unwrap();crates/apollo_batcher/src/commitment_manager/state_committer.rs line 107 at r2 (raw file):
/// Panics on unrecoverable errors. async fn handle_task_error(error: CommitterClientError) {
is this more descriptive?
Suggestion:
log_errors_and_maybe_panicb4d8a3a to
c1961fb
Compare
2fd00f8 to
38f59ce
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoavGrs made 4 comments.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @amosStarkware).
crates/apollo_batcher/src/commitment_manager/state_committer.rs line 50 at r2 (raw file):
Previously, amosStarkware wrote…
the original name was before we decided to add revert tasks. please also rename
perform_commitment_task.
you can also add a TODO on me if it's a lot of work
Done.
crates/apollo_batcher/src/commitment_manager/state_committer.rs line 58 at r2 (raw file):
Previously, amosStarkware wrote…
I don't think we need a config here:
the tasks channel may grow very large if the committer is lagging behind the batcher.
but the results channel shouldn't grow very large. I'll keep this discussion open in case Dori thinks otherwise.
please change to try_send, match on errors (for more descriptive panic messages), and log when a result is added. similar to this
Done.
crates/apollo_batcher/src/commitment_manager/state_committer.rs line 107 at r2 (raw file):
Previously, amosStarkware wrote…
is this more descriptive?
Done.
crates/apollo_batcher/src/commitment_manager/state_committer.rs line 49 at r2 (raw file):
impl StateCommitter { /// Repeatedly performs any task in the channel.
Done.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @amosStarkware).
amosStarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amosStarkware reviewed 1 file and all commit messages, made 1 comment, and resolved 4 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
crates/apollo_batcher/src/commitment_manager/state_committer.rs line 67 at r3 (raw file):
} Err(TrySendError::Full(_)) => panic!("Results channel is full."), Err(err) => panic!("Failed to send results. error: {err}"),
please add the results height to the error messages
Code quote:
Err(TrySendError::Full(_)) => panic!("Results channel is full."),
Err(err) => panic!("Failed to send results. error: {err}"),
amosStarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amosStarkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yoavGrs).
-- commits line 2 at r3:
Also - please add a unit test.
Can also add a TODO, if you're short on time
Code quote:
- 38f59ce: apollo_batcher: perform commitment tasks
amosStarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amosStarkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware).
crates/apollo_batcher/src/commitment_manager/state_committer.rs line 122 at r9 (raw file):
Previously, dorimedini-starkware wrote…
still relevant?
Maybe change to:
// TODO(Amos): If the task performer panics, the Batcher will panic when it attempts to add tasks to the commitment manager. In this case, print an informative message.
a582fd9 to
881dce6
Compare
c7dec77 to
be58b49
Compare
|
Artifacts upload workflows: |
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 6 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware).
be58b49 to
49937fa
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware).
49937fa to
fc6eeaa
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware).
amosStarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amosStarkware reviewed 7 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yoavGrs).
e4c4f95 to
67c7585
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware).
crates/apollo_batcher/src/batcher.rs line 1162 at r16 (raw file):
for commitment_task_output in commitment_results.into_iter() { let height = commitment_task_output.height; info!("Writing commitment results to storage for height {}.", height);
non-blocking
Suggestion:
info!("Writing commitment results to storage for height {height}.");5df3bd2 to
ba57132
Compare
67c7585 to
0150673
Compare
ba57132 to
6f0400c
Compare
0150673 to
feb1959
Compare
6f0400c to
c7cd8e0
Compare
feb1959 to
2dcf469
Compare
c7cd8e0 to
090af3f
Compare
c8e17ba to
c476ae8
Compare
c476ae8 to
2fb3447
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware).

No description provided.