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

refactor(remote_timeline_client): Split deletion into unlinking + deletion #5645

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

koivunej
Copy link
Member

Quest: #4745. Prerequisite for #4938. Original #4938 (comment).

The new Layer implementation has so far been using RemoteTimelineClient::schedule_layer_file_deletion from Layer::drop but it was noticed that this could mean that the L0s compaction wanted to remove could linger in the index part for longer time or be left there for longer time. Solution is to split the RemoteTimelineClient::schedule_layer_file_deletion into two parts:

  • unlinking from index_part.json, to be called from end of compaction and gc
  • scheduling of actual deletions, to be called from Layer::drop

The added methods are added unused.

@koivunej koivunej requested a review from jcsp October 24, 2023 15:00
@koivunej koivunej requested a review from a team as a code owner October 24, 2023 15:00
@koivunej koivunej requested review from hlinnaka and removed request for a team October 24, 2023 15:00
@koivunej
Copy link
Member Author

koivunej commented Oct 24, 2023

I did start to wonder how is that the compaction is sane without a single add+remove operation. Perhaps we have just been lucky enough? Timeline::compact for L0 => L1 compaction will always do:

  1. upload layer(s)
  2. remove layers <-- schedules index part upload after inmem changes but before deletions
  3. (there's a later schedule_index_upload_for_file_changes, but now there is nothing pending)

So not lucky at all, the multiple method call interface is just essentially giving this batched operation. Between the steps and loop around step 1 above there could be other interactions, and that would be fine even with these changes.

@koivunej koivunej requested review from problame and removed request for hlinnaka October 24, 2023 15:24
@github-actions
Copy link

2340 tests run: 2224 passed, 0 failed, 116 skipped (full report)


Code coverage (full report)

  • functions: 53.5% (8564 of 16012 functions)
  • lines: 80.9% (49773 of 61507 lines)

The comment gets automatically updated with the latest test results
c292bc7 at 2023-10-24T15:46:30.517Z :recycle:

@koivunej koivunej merged commit 4ae2d13 into main Oct 25, 2023
39 checks passed
@koivunej koivunej deleted the split_rtc_deletion branch October 25, 2023 12:01
koivunej added a commit that referenced this pull request Oct 25, 2023
a single operation instead of N uploads and 1 deletion scheduling with
write(layer_map) lock releasing in the between. Compaction update will
make for a much better place to change how the operation will change in
future compared to more general file based operations.

builds upon #5645. solves the problem of difficult to see hopeful
correctness w.r.t. other `index_part.json` changing operations.

Co-authored-by: Shany Pozin <shany@neon.tech>
koivunej added a commit that referenced this pull request Oct 26, 2023
…#4938)

Implement a new `struct Layer` abstraction which manages downloadness
internally, requiring no LayerMap locking or rewriting to download or
evict providing a property "you have a layer, you can read it". The new
`struct Layer` provides ability to keep the file resident via a RAII
structure for new layers which still need to be uploaded. Previous
solution solved this `RemoteTimelineClient::wait_completion` which lead
to bugs like #5639. Evicting or the final local deletion after garbage
collection is done using Arc'd value `Drop`.

With a single `struct Layer` the closed open ended `trait Layer`, `trait
PersistentLayer` and `struct RemoteLayer` are removed following noting
that compaction could be simplified by simply not using any of the
traits in between: #4839.

The new `struct Layer` is a preliminary to remove
`Timeline::layer_removal_cs` documented in #4745.

Preliminaries: #4936, #4937, #5013, #5014, #5022, #5033, #5044, #5058,
#5059, #5061, #5074, #5103, epic #5172, #5645, #5649. Related split off:
#5057, #5134.
koivunej added a commit that referenced this pull request Feb 27, 2024
Not allowing evicting wanted deleted layers is something I've forgotten
to implement on #5645. This PR makes it possible to evict such layers,
which should reduce the amount of hanging evictions.

Fixes: #6928

Co-authored-by: Christian Schwarz <christian@neon.tech>
koivunej added a commit that referenced this pull request Feb 27, 2024
Not allowing evicting wanted deleted layers is something I've forgotten
to implement on #5645. This PR makes it possible to evict such layers,
which should reduce the amount of hanging evictions.

Fixes: #6928

Co-authored-by: Christian Schwarz <christian@neon.tech>
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