Skip to content

refactor(event cache): clarify warning when a pinned event couldn't be loaded#6405

Merged
bnjbvr merged 1 commit intomainfrom
bnjbvr/clarify-warning-pinned-events
Apr 2, 2026
Merged

refactor(event cache): clarify warning when a pinned event couldn't be loaded#6405
bnjbvr merged 1 commit intomainfrom
bnjbvr/clarify-warning-pinned-events

Conversation

@bnjbvr
Copy link
Copy Markdown
Member

@bnjbvr bnjbvr commented Apr 2, 2026

The previous warning could trigger with abnormal numbers, since there could be related events applying to a pinned event, the loaded_events list could be bigger than the list of pinned_event_ids, due to the eager flattening. This patch makes it display the correct number, by postponing the flattening after the warning.

  • I've documented the public API Changes in the appropriate CHANGELOG.md files.
  • This PR was made with the help of AI.

@bnjbvr bnjbvr requested a review from a team as a code owner April 2, 2026 07:47
@bnjbvr bnjbvr requested review from stefanceriu and removed request for a team April 2, 2026 07:47
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.86%. Comparing base (32e9626) to head (8d2ab5a).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6405      +/-   ##
==========================================
+ Coverage   89.85%   89.86%   +0.01%     
==========================================
  Files         378      378              
  Lines      103384   103389       +5     
  Branches   103384   103389       +5     
==========================================
+ Hits        92893    92911      +18     
+ Misses       6928     6907      -21     
- Partials     3563     3571       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good finding, thanks. I am suggesting a way to avoid the double allocation, not tested though.

Comment on lines +504 to 505
// Get rid of error results.
.flat_map(stream::iter)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good finding. However, we now allocate twice (with collect) instead of once.

We could use StreamExt::inspect() to count the number of pinned events instead of doing this intermediate collect. Something like:

let mut number_of_loaded_events = 0;

let loaded_events: Vec<Event> =
    stream::iter()
        .buffer_unordered(max_concurrent_requests)
        .flat_map(stream::iter)
        .inspect(|_| number_of_loaded_events += 1)
        .flat_map(stream::iter)
        .collect();

I think that could work.

Thoughts?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice, didn't know about that particular StreamExt method, thanks!

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 2, 2026

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing bnjbvr/clarify-warning-pinned-events (8d2ab5a) with main (32e9626)

Open in CodSpeed

…e loaded

The previous warning could trigger with abnormal numbers, since there
could be related events applying to a pinned event, the `loaded_events`
list could be bigger than the list of `pinned_event_ids`, due to the
eager flattening. This patch makes it display the correct number, by
postponing the flattening after the warning.
@bnjbvr bnjbvr force-pushed the bnjbvr/clarify-warning-pinned-events branch from 23cad1f to 8d2ab5a Compare April 2, 2026 10:23
@bnjbvr bnjbvr enabled auto-merge (rebase) April 2, 2026 10:23
@bnjbvr bnjbvr merged commit a1157d2 into main Apr 2, 2026
52 checks passed
@bnjbvr bnjbvr deleted the bnjbvr/clarify-warning-pinned-events branch April 2, 2026 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants