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

load_layer_map: schedule deletions for any future layers #5103

Merged
merged 4 commits into from
Aug 28, 2023

Conversation

koivunej
Copy link
Member

Unrelated fixes noticed while integrating #4938.

  • We leak future layers in remote storage
  • We schedule extra index_part uploads if layer name to be removed was not actually present

@github-actions
Copy link

github-actions bot commented Aug 25, 2023

1624 tests run: 1548 passed, 0 failed, 76 skipped (full report)


Flaky tests (4)

Postgres 15

  • test_single_branch_get_tenant_size_grows: release
  • test_get_tenant_size_with_multiple_branches: release
  • test_timeline_physical_size_post_checkpoint[local_fs]: release

Postgres 14

  • test_get_tenant_size_with_multiple_branches: release
The comment gets automatically updated with the latest test results
ecf85de at 2023-08-27T09:23:14.604Z :recycle:

@koivunej koivunej changed the title remote_timeline_client: smaller adjustments load_layer_map: schedule deletions for any future layers Aug 25, 2023
Copy link
Contributor

@problame problame left a comment

Choose a reason for hiding this comment

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

Can't see anything wrong with this OTOH.

I wonder if we should just delete the local files instead of rename-to-backup.

@koivunej koivunej merged commit fbcd174 into main Aug 28, 2023
@koivunej koivunej deleted the remove_remote_layer_9 branch August 28, 2023 07:51
arpad-m pushed a commit that referenced this pull request Aug 28, 2023
Unrelated fixes noticed while integrating #4938.

- Stop leaking future layers in remote storage
- We schedule extra index_part uploads if layer name to be removed was
not actually present
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.
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