This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Handling timers for repeat dispute participation requests #6901
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
BradleyOlson64
added
A0-please_review
BradleyOlson64
requested review from
tdimitrov and
mrcnski
and removed request for
tdimitrov and
mrcnski
mrcnski
reviewed
Mar 17, 2023
mrcnski
reviewed
Mar 17, 2023
mrcnski
reviewed
Mar 17, 2023
eskimor
reviewed
Mar 17, 2023
| timer.stop_and_discard(); | ||
| } | ||
| } else { | ||
| self.best_effort.insert(comparator, req); |
There was a problem hiding this comment.
You can avoid the double lookup via entry.
There was a problem hiding this comment.
Maybe I'm missing something obvious, but here's the problem I see there. The intuitive pattern is something like:
self.priority.entry(comparator).or_insert(req);
But this pattern drops req without calling stop_and_discard() on its timer if there was an older request. This sends bad data to our metrics. I don't see a way to call stop_and_discard() without using a double lookup. 🤷♂️
There was a problem hiding this comment.
Can't you use or_insert_with to execute timer.stop_and_discard() first and then insert req?
There was a problem hiding this comment.
you can also always pattern match on the Entry constructors and do whatever you need to do. See here for an example.
There was a problem hiding this comment.
Your solution worked @eskimor Much appreciated!
eskimor
approved these changes
Mar 20, 2023
|
bot merge |
ordian
added a commit
that referenced
this pull request
Mar 21, 2023
* master: kusama: enable dispute slashes (#5974) Introduce OpenGov into Polkadot (#6701) introduce new well known key (#6915) [CI] Add bootnode checking CI jobs (#6889) Bump parity-db (#6921) Handling timers for repeat dispute participation requests (#6901) [Companion #13634] keystore overhaul (iter2) (#6913) tweak some pattern matches to address a new clippy warning Bump ci-linux image for rust 1.68 Revert "Update orchestra to the recent version (#6854)" (#6916) Deprecate Currency: Companion for #12951 (#6780) changelog: template fixup (#6907) [Companion #13615] Keystore overhaul (#6892) update weights (#6897) Fix approval voting test (#6898) parachains-runtime: Less cloning! (#6896) Testing Reversion Speed on Dispute Concluded Against (#6880) remove duplicated arm and fix version index (#6884)
ordian
added a commit
that referenced
this pull request
Mar 21, 2023
* master: kusama: enable dispute slashes (#5974) Introduce OpenGov into Polkadot (#6701) introduce new well known key (#6915) [CI] Add bootnode checking CI jobs (#6889) Bump parity-db (#6921) Handling timers for repeat dispute participation requests (#6901) [Companion #13634] keystore overhaul (iter2) (#6913) tweak some pattern matches to address a new clippy warning Bump ci-linux image for rust 1.68 Revert "Update orchestra to the recent version (#6854)" (#6916) Deprecate Currency: Companion for #12951 (#6780) changelog: template fixup (#6907) [Companion #13615] Keystore overhaul (#6892) update weights (#6897) Fix approval voting test (#6898) parachains-runtime: Less cloning! (#6896) Testing Reversion Speed on Dispute Concluded Against (#6880) remove duplicated arm and fix version index (#6884)
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.
This is a follow up to #6838 aiming to make the metric
polkadot_parachain_dispute_participation_pipeline_durationsmore accurate.Previously, if multiple participation requests were made for the same candidate then the older request would simply be dropped. But dropping the older request meant dropping its request timer as well. The pipeline duration metric only works properly if each request timer is dropped after we've participated in its corresponding dispute.
These changes fix the issue, keeping the oldest timer for each participation request alive and disposing of newer duplicate timers using
stop_and_discard()