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(sdk): Introduce LinkedChunkUpdate #3281

Merged
merged 7 commits into from
May 2, 2024

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Mar 28, 2024

This patch introduces LinkedChunkUpdate and LinkedChunk::updates(&mut self) -> &mut Vec<LinkedChunkUpdate>. It's a new mechanism that is used by LinkedChunk in order to get a list of all updates that happened in a LinkedChunk. This is the fundamental piece to write a persistent storage for LinkedChunk, and thus for EventCache, see #3280 to learn more.


@Hywan Hywan mentioned this pull request Mar 28, 2024
37 tasks
@Hywan Hywan force-pushed the feat-sdk-linked-chunk-listener branch 2 times, most recently from 6453764 to abe2206 Compare March 28, 2024 16:01
@Hywan Hywan force-pushed the feat-sdk-linked-chunk-listener branch from 665eca2 to 20398ea Compare April 29, 2024 13:07
Hywan added 2 commits April 29, 2024 15:08
This patch changes the signature of `LinkedChunk<Item, Gap, const
CHUNK_CAPACITY = usize>` to `LinkedChunk<const CHUNK_CAPACITY = usize,
Item, Gap>`. It allows to add more generic parameters if needed, without
conflicting with generic constants.
@Hywan Hywan force-pushed the feat-sdk-linked-chunk-listener branch from 20398ea to 93b6134 Compare April 29, 2024 13:27
@Hywan Hywan marked this pull request as ready for review April 29, 2024 13:27
@Hywan Hywan requested a review from a team as a code owner April 29, 2024 13:27
@Hywan Hywan requested review from andybalaam and bnjbvr and removed request for a team and andybalaam April 29, 2024 13:27
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of async + Result infecting the whole linked chunk API, could we find another way that would help separate concerns and result in a more robust design, please?

crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
@Hywan Hywan force-pushed the feat-sdk-linked-chunk-listener branch from 93b6134 to 88bde57 Compare May 1, 2024 13:44
Copy link

codecov bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 74.80916% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 83.31%. Comparing base (16eb449) to head (443647a).
Report is 28 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk/src/event_cache/linked_chunk.rs 75.19% 32 Missing ⚠️
crates/matrix-sdk/src/event_cache/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3281      +/-   ##
==========================================
- Coverage   83.65%   83.31%   -0.35%     
==========================================
  Files         242      242              
  Lines       25012    25160     +148     
==========================================
+ Hits        20924    20962      +38     
- Misses       4088     4198     +110     

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

@Hywan Hywan force-pushed the feat-sdk-linked-chunk-listener branch from 88bde57 to 10e2fbd Compare May 1, 2024 14:56
@Hywan Hywan changed the title feat(sdk): Introduce LinkedChunkListener feat(sdk): Introduce LinkedChunkUpdate May 1, 2024
@Hywan Hywan force-pushed the feat-sdk-linked-chunk-listener branch 2 times, most recently from cbc4e89 to ae6f23a Compare May 1, 2024 15:13
@bnjbvr bnjbvr self-requested a review May 1, 2024 16:04
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Looking great! Small request to make the update_history optional, and can you also clean the PR history before merging, please? Thanks!

crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/event_cache/linked_chunk.rs Outdated Show resolved Hide resolved
@Hywan Hywan force-pushed the feat-sdk-linked-chunk-listener branch from 1ed1c1a to 2b6c4db Compare May 2, 2024 12:25
Hywan added 5 commits May 2, 2024 14:30
This patch creates the `LinkedChunkListener` trait.

This patch also updates `LinkedChunk` to be able to use a
`LinkedChunkListener`.
This patch is a turn around about the `LinkedChunkListener`. Many
patches have been removed because `LinkedChunkListener` needed to
support I/O, so errors and async code. The whole code was affected by
that, resulting in a complex API. The idea of this patch is to decoupled
this. Here is how.

First off, `LinkedChunkListener` is removed. So it's one less generic
parameter on `LinkedChunk`. It's also one less trait, so less
implementations.

Second, now `LinkedChunk` accumulates/collects all “updates” under the
form of a new enum `LinkedChunkUpdate`. These updates can be read with
`LinkedChunk::updates(&mut self) -> &Vec<LinkedChunkUpdate>`. The reader
can simply read them, or even drain them. The reader is responsible
to handle these updates and to dispatch them in a storage or whatever.
`LinkedChunk` is no longer responsible to do that, removing the need to
support errors and to be async.

Third, the simplification has led to an optimisation by introducing a
new type `LinkedChunkLinks`. The documentation explains what it does
and why it was needed. The benefit of this type is: it doesn't increase
the size of `LinkedChunk`, but it simplifies the code: no more `Arc`,
no more `Mutex` (it was required because with I/O and async), no more
borrow checker trick, and the code stays as safe as before.
This patch makes the `LinkedChunk::update_history` field optional,
so that it doesn't require the user to drain it to avoid eating the
universe.

The `new` constructor disabled the update history, the
`new_with_update_history` enables it.
@Hywan Hywan force-pushed the feat-sdk-linked-chunk-listener branch from 2b6c4db to 443647a Compare May 2, 2024 12:30
@Hywan Hywan enabled auto-merge May 2, 2024 12:30
@Hywan Hywan merged commit 67e2842 into matrix-org:main May 2, 2024
35 checks passed
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