[cli] Increase REST endpoint timeout to 30s #4628
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
We saw bunch of timeouts with 10s timeout during loadtests, and then the experience is confusing - transcation actually goes through, but that is not clear
|
I think this is long enough that many users will think something went wrong / the request hung. Could we instead decrease the expiration time of the requests the CLI sends and then decrease this timeout accordingly? Alternatively it would be okay to do this if the CLI made it clear that it was still waiting for the request, e.g. with some logging to stderr. |
|
Okay we spoke offline, I was misunderstanding how time is tracked on chain (blockchain time vs absolute time, which can differ under high load). This makes sense then. |
banool
approved these changes
Sep 29, 2022
zekun000
approved these changes
Sep 29, 2022
sitalkedia
reviewed
Sep 29, 2022
|
|
||
| /// Connection timeout in seconds, used for the REST endpoint of the fullnode | ||
| #[clap(long, default_value = "30")] | ||
| pub connection_timeout_s: u64, |
There was a problem hiding this comment.
Can you clarify where is this set to 10s default today?
igor-aptos
commented
Sep 29, 2022
| @@ -823,7 +831,10 @@ impl RestOptions { | |||
| } | |||
|
|
|||
| pub fn client(&self, profile: &str) -> CliTypedResult<Client> { | |||
| Ok(Client::new(self.url(profile)?)) | |||
There was a problem hiding this comment.
here, implementation is:
pub fn new(base_url: Url) -> Self {
Self::new_with_timeout(base_url, Duration::from_secs(10))
}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
sitalkedia
approved these changes
Sep 29, 2022
✅ Forge suite
|
This was referenced Sep 30, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We saw bunch of timeouts with 10s timeout during loadtests, and then the experience is confusing - transaction actually goes through, but that is not clear.
We used 30s timeout for txn emitter, and that worked well.
together with #4456, CLI experience should be better now
Description
Test Plan
This change is