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.

Sign up for GitHub

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

Jump to bottom

prepare to move timeouts and cancellation handling to remote_storage #6696

Merged
merged 16 commits into from
Feb 9, 2024
Merged

prepare to move timeouts and cancellation handling to remote_storage #6696

merged 16 commits into from
Feb 9, 2024

Conversation

koivunej
Copy link
Member

@koivunej koivunej commented Feb 8, 2024 •
edited
Loading

This PR is preliminary cleanups and refactoring around remote_storage for next PR which will move the timeouts and cancellation into remote_storage.

Summary:

  • smaller drive-by fixes
  • code simplification
  • refactor common parts like DownloadError::is_permanent
  • align error types with RemoteStorage::list_* to use more download_retry helper

Cc: #6096

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 8, 2024 •
edited
Loading

2418 tests run: 2299 passed, 0 failed, 119 skipped (full report)

Flaky tests (2)

Postgres 16

  • test_crafted_wal_end[last_wal_record_crossing_segment]: debug

Postgres 14

  • test_pg_regress[None]: debug

Code coverage (full report)

  • functions: 55.0% (11800 of 21465 functions)
  • lines: 82.3% (66029 of 80275 lines)
The comment gets automatically updated with the latest test results
4118408 at 2024-02-09T12:39:53.755Z :recycle:

koivunej added 15 commits February 8, 2024 22:57
@koivunej
fix: accidential tokio::io::copy
0956f61
@koivunej
refactor: localfs test: read_and_check_metadata
2635165
@koivunej
refactor: move out PermitCarrying
619ac01
@koivunej
refactor: remove extra derive-default
3d2ef05
@koivunej
refactor(local_fs): remove conditional take
e140f3b 
just check the length on disk first.
@koivunej
refactor(s3_bucket): unnest the ok branch
825c0c5
@koivunej
refactor(s3_bucket): remove needless cloning
f253f33
@koivunej
refactor: move the constants above structs and whatnot
611ceb7
@koivunej
drive-by fix: avoid cloning CancellationToken for queries
0a0f5b9
@koivunej
refactor: DownloadError::is_permanent
3eebdf9
@koivunej
fix(list): confused return types
c122ce9
@koivunej
fix(rtc): reuse download_retry now that list types are fixed
2bc542c
@koivunej
refactor: remove cancellation token cloning
1f9f942
@koivunej
fix: context copypaste error in error msg
6690323
@koivunej
fix(rtc): use one cancel token
ae72c8a
@koivunej koivunej force-pushed the move_timeout_cancel2 branch from 3e8cf0b to ae72c8a Compare February 9, 2024 07:12
@koivunej koivunej changed the title move timeouts and cancellation handling to remote_storage prepare to move timeouts and cancellation handling to remote_storage Feb 9, 2024
@koivunej koivunej mentioned this pull request Feb 9, 2024
Merged
@koivunej koivunej marked this pull request as ready for review February 9, 2024 08:05
@koivunej koivunej requested a review from a team as a code owner February 9, 2024 08:05
@koivunej koivunej requested review from VladLazar and removed request for a team February 9, 2024 08:05
VladLazar
VladLazar approved these changes Feb 9, 2024
View reviewed changes
Copy link
Contributor

@VladLazar VladLazar left a comment

Choose a reason for hiding this comment

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

Looks good to me - just nits

pageserver/src/tenant/remote_timeline_client/download.rs Show resolved Hide resolved
libs/remote_storage/src/local_fs.rs Outdated Show resolved Hide resolved
libs/remote_storage/src/local_fs.rs Show resolved Hide resolved
@koivunej koivunej enabled auto-merge (squash) February 9, 2024 12:34
@koivunej koivunej merged commit eb919ca into main Feb 9, 2024
49 checks passed
@koivunej koivunej deleted the move_timeout_cancel2 branch February 9, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladLazar VladLazar VladLazar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@koivunej @VladLazar