Skip to content
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

feat(linked chunk): add a way to reconstruct a linked chunk from its raw representation #4318

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Nov 25, 2024

What it says on the tin. Extracted from #4308, with extra testing and better error handling.

In particular, we must be careful that the passed set of chunks form a linked list, and not a looser kind of a graph.

Two things I'm not very sure about:

  • should we have a dedicated pass only to check the internal links are consistent? I think what I've done is equivalent and correct (i.e.: manually check the previous, remove from self.chunks while constructing, ensure self.chunks is empty after iterating), but an explicit pass might be more straightforward and clearer in its intent.
  • is it fine that Ends::last is always set? It's not clear to me why it should be optional, since its users will either take it, or take first when it's not set.

@bnjbvr bnjbvr requested a review from Hywan November 25, 2024 15:46
@bnjbvr bnjbvr requested a review from a team as a code owner November 25, 2024 15:46
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 16 lines in your changes missing coverage. Please review.

Project coverage is 85.07%. Comparing base (75d7d07) to head (e8bfd38).

Files with missing lines Patch % Lines
...ates/matrix-sdk-common/src/linked_chunk/builder.rs 80.95% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4318      +/-   ##
==========================================
+ Coverage   85.05%   85.07%   +0.01%     
==========================================
  Files         275      276       +1     
  Lines       30309    30397      +88     
==========================================
+ Hits        25780    25859      +79     
- Misses       4529     4538       +9     

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

Copy link
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.

Okay, I've left a lot of comments but (!) I like the strategy you've taken: I think it's a valid one!

I am hesitant to ask the modify the build() method to run 2 phases:

  1. check data consistency,
  2. build the actual LinkedChunk

I suppose you've merged the 2 phases in a single one for performance reason, but it makes it a bit more complex to understand. Not a big deal though. I wonder if it has any performance impact in reality, as we may not build huge LinkedChunk, vs. having super clear and easy-to-understand code.

crates/matrix-sdk-common/src/linked_chunk/builder.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/builder.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/builder.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/builder.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-common/src/linked_chunk/builder.rs Outdated Show resolved Hide resolved
@bnjbvr bnjbvr force-pushed the bnjbvr/linked-chunk-rebuilder branch from 1215b8b to 0504265 Compare November 26, 2024 17:10
@bnjbvr bnjbvr requested a review from Hywan November 26, 2024 17:10
@bnjbvr
Copy link
Member Author

bnjbvr commented Nov 26, 2024

Alright, please take another look. I'm not super satisfied with the splitting of the building into two parts (you'll see that some checks are redundant, with some branches that can't be hit, because of this), but this is likely semi-temporary code anyways.

The one thing I'm tempted to do is to get rid of all the error types and keep a single one "MalformedList", because the detail of what the error is only matters for the testing purposes…

@bnjbvr bnjbvr force-pushed the bnjbvr/linked-chunk-rebuilder branch from 0504265 to e8bfd38 Compare November 26, 2024 17:12
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.

3 participants