beefy: Increment metric and add extra log details #5075
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
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
lexnv
added
A1-insubstantial
acatangiu
reviewed
Jul 19, 2024
Comment on lines
101
to
105
| /// Returns the next peer from the peers cache, if available. | ||
| /// | ||
| /// If the optional block number if provided, the peers cache is | ||
| /// repopulated from the live peers that have voted on higher block numbers. | ||
| fn try_next_peer(&mut self, reset_cache: Option<NumberFor<B>>) -> Option<PeerId> { |
There was a problem hiding this comment.
This extra Option on this fn makes the code harder to read and reason about.
Is it making any difference in practice, at least? From what I see, all it achieves is locking once for longer rather than lock, release, lock.
Is this improving the behavior or lowering lock thrashing?
There was a problem hiding this comment.
Indeed, don't think the double-lock is having a huge impact on performance :) Just noticed it while digging for some metrics
When we lock release lock, there might be a chance that the peers we have in cache will disconnect in between the locks (and also get propagated to live peers). But I think we have enough info with the metrics and extra logs to catch these.
TLDR; lets keep the code simple :)
This reverts commit 7c77d4e. Signed-off-by: Alexandru Vasile <[email protected]>
lexnv
changed the title
acatangiu
approved these changes
Jul 19, 2024
bkchr
approved these changes
Jul 20, 2024
TarekkMA
pushed a commit
to moonbeam-foundation/polkadot-sdk
that referenced
this pull request
Aug 2, 2024
This PR increments the beefy metric wrt no peers to query justification from. The metric is incremented when we submit a request to a known peer, however that peer failed to provide a valid response, and there are no further peers to query. While at it, add a few extra details to identify the number of active peers and cached peers, together with the request error Part of: - paritytech#4985 - paritytech#4925 --------- Signed-off-by: Alexandru Vasile <[email protected]>
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.
This PR increments the beefy metric wrt no peers to query justification from.
The metric is incremented when we submit a request to a known peer, however that peer failed to provide a valid response, and there are no further peers to query.
While at it, add a few extra details to identify the number of active peers and cached peers, together with the request error
Part of: