-
Notifications
You must be signed in to change notification settings - Fork 5
feature(collator): append cumulative statistics after new master block #768
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: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #768 +/- ##
==========================================
+ Coverage 47.38% 47.42% +0.03%
==========================================
Files 296 297 +1
Lines 57500 58068 +568
Branches 57500 58068 +568
==========================================
+ Hits 27249 27540 +291
- Misses 28847 29119 +272
- Partials 1404 1409 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7528aa9
to
9a3bc29
Compare
9a3bc29
to
e32b0c8
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.
Pull Request Overview
The PR introduces cumulative statistics support after processing a new master block, along with type and trait renaming to support the new bounded range mechanism. Key changes include:
- Updating types from QueueShardRange to QueueShardBoundedRange and adding the Bound enum.
- Adding a new field (prev_mc_block_id) to McData and updating cumulative statistics processing logic.
- Renaming traits (e.g. ShardDescriptionExt to ShardDescriptionShortExt) and propagating new range types throughout the collator modules.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
collator/src/types.rs | Added prev_mc_block_id field and updated error handling in state load |
collator/src/queue_adapter.rs | Changed parameter types to use bounded range types |
collator/src/manager/* | Renamed trait usages to use ShardDescriptionShortExt |
collator/src/internal_queue/types.rs | Introduced new Bound enum and QueueShardBoundedRange with conversion |
collator/src/collator/* | Updated cumulative statistics logic, debug printing, and state fields |
collator/src/collator/tests/* | Updated tests to account for new fields and parameters |
collator/src/collator/do_collate/* | Integrated new part_stat_ranges logic and improved diff loading |
Comments suppressed due to low confidence (1)
collator/src/collator/types.rs:1423
- New cumulative statistics update logic is introduced here, but the PR metadata indicates no unit test coverage. It is recommended to add tests to validate this behavior.
pub fn update_processed_to_by_partitions(&mut self, new_pt: FastHashMap<ShardIdent, (bool, ProcessedToByPartitions)>) {
Bound::Included(value) => value, | ||
Bound::Excluded(value) => value.next_value(), | ||
}; | ||
|
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.
The conversion logic for QueueShardBoundedRange to QueueShardRange uses next_value() for Bound::Included on the 'to' side, which contrasts with the handling of the 'from' side. Consider adding an inline comment to clarify the intent of these boundary conversions.
// For the 'to' side, Bound::Included is converted using next_value() to ensure | |
// the range excludes the upper boundary. This contrasts with the 'from' side, | |
// where Bound::Included directly uses the value to include the lower boundary. |
Copilot uses AI. Check for mistakes.
cx.mc_state_gen_lt, | ||
&cx.mc_top_shards_end_lts.iter().copied().collect(), | ||
)?; | ||
|
||
cumulative_stats_just_loaded = true; |
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 it correct to set cumulative_stats_just_loaded = true
when cumulative stats is partially appended?
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.
cumulative_stats_just_loaded
removed for enriched data because cumulative statistics is already has reduced remaining stats
@@ -623,6 +623,7 @@ impl<V: InternalMessageValue> TestCollator<V> { | |||
anchors_cache, | |||
is_first_block_after_prev_master, | |||
cumulative_stats_calc_params: cumulative_stats_calc_params.clone(), | |||
part_stat_ranges: None, |
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.
Better to use new logic in these tests
collator/src/collator/types.rs
Outdated
self.truncate_range(r); | ||
} | ||
|
||
self.dirty = true; |
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.
Can we avoid recalculation?
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.
Refactored. dirty
fully removed
collator/src/collator/types.rs
Outdated
// TODO should remove full stat by shard if it doesn't exist in new_pt? | ||
|
||
self.all_shards_processed_to_by_partitions = new_pt; | ||
self.dirty = true; |
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.
Can we avoid recalculation?
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.
Refactored. dirty
fully removed
0a329a7
to
b02b785
Compare
b5938ef
to
14c12aa
Compare
8b6cda0
to
e1864af
Compare
collator/src/collator/types.rs
Outdated
@@ -1279,6 +1280,9 @@ pub struct QueueStatisticsWithRemaning { | |||
} | |||
|
|||
pub struct CumulativeStatistics { | |||
/// Cumulative statistics created for this shard. When this shard reads statistics, it decrements `remaining messages` |
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.
/// Cumulative statistics created for this shard. When this shard reads statistics, it decrements `remaining messages` | |
/// Cumulative statistics created for this shard. When reader reads messages, it decrements `remaining messages` |
collator/src/collator/types.rs
Outdated
@@ -1279,6 +1280,9 @@ pub struct QueueStatisticsWithRemaning { | |||
} | |||
|
|||
pub struct CumulativeStatistics { | |||
/// Cumulative statistics created for this shard. When this shard reads statistics, it decrements `remaining messages` | |||
/// Another shard can decrements the statistics remaining only by using `update_processed_to_by_partitions` |
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.
/// Another shard can decrements the statistics remaining only by using `update_processed_to_by_partitions` | |
/// Another shard stats can be decremented only by calling `update_processed_to_by_partitions` |
e1864af
to
167dea2
Compare
NODE CONFIGURATION MODEL CHANGES
NO
BLOCKCHAIN CONFIGURATION MODEL CHANGES
NO
SPECIAL DEPLOYMENT ACTIONS
Not Required
PERFORMANCE IMPACT
Now statistics is reusing from previous collation and full reload statistics doesn't necessary
Metrtics
tycho_internal_queue_separated_statistics_load_time
should looks better1-255 10k + transfers 20k:

TESTS
Unit Tests
No covarage
Network Tests
No covarage
Manual Tests
1-255 10k + transfers 20k