-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
Retry failed requests in benchmark #3119
Conversation
48cccb7
to
ca61019
Compare
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.
Thanks! Made a few comments, mostly stylistic.
// TODO (GAS-LEAK): How do we add this gas back in the pool? | ||
NextOp::Response(None) | ||
// eprintln!("{}", sui_err); | ||
NextOp::Retry(Box::new((tx, counter_id, owner))) | ||
} | ||
_ => { | ||
// eprintln!("error"); |
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.
I'd nix those prints or make the logs while we're at it.
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.
done
@@ -236,7 +237,7 @@ async fn main() { | |||
make_counter_increment_transaction(gas.1 .0, package_ref, counter_id); | |||
let res = qd | |||
.execute_transaction(ExecuteTransactionRequest { | |||
transaction: tx, | |||
transaction: tx.clone(), |
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.
Is this necessary?
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.
yes, because we are moving the tx object later on
Err(_sui_err) => { | ||
//eprintln!("{}", _sui_err); | ||
NextOp::Retry(b) | ||
} | ||
_ => { | ||
// eprintln!("error"); | ||
NextOp::Retry(b) | ||
} |
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.
Those can probably de made more compact.
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.
hopefully addresses
.map(move |res| { | ||
match res { | ||
Ok(ExecuteTransactionResponse::EffectsCert(result)) => { | ||
let (_, effects) = *result; |
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.
You can put this destructuring pattern in the line right above.
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.
I tried, but looks like i cannot destruct the Box<> inside the EffectsCert
e8367e1
to
7dc85db
Compare
7dc85db
to
f7f632d
Compare
We can retry the failed requests and expect to get effects from the transaction digest if the previous one went through or submit a new one again