Page MenuHomePhabricator

[native] Notify backup uploader after compaction creation
ClosedPublic

Authored by michal on Jan 29 2024, 2:36 AM.
Tags
None
Referenced Files
F3392333: D10858.diff
Sat, Nov 30, 8:15 AM
Unknown Object (File)
Wed, Nov 27, 11:43 AM
Unknown Object (File)
Wed, Nov 13, 4:55 AM
Unknown Object (File)
Wed, Nov 13, 4:55 AM
Unknown Object (File)
Wed, Nov 13, 4:55 AM
Unknown Object (File)
Wed, Nov 13, 4:55 AM
Unknown Object (File)
Wed, Nov 13, 4:55 AM
Unknown Object (File)
Fri, Nov 1, 9:05 PM
Subscribers

Details

Summary

After JS schedules new backup compaction creation we want to keep track of the promise_id so we can resolve/reject it after the operation. But the logic is split Rust -> C++ (compaction creation) -> Rust (upload to backup service) so we need to keep a map from backupID to the promise id.

Test Plan

Tested with the later diff but things that touch this code in particular:

  • tested that the promise resolved after successful upload
  • tested that the promise rejected with a correct message after an error on the Rust side
  • tested that the promise rejected with a correct message after an error on the C++ side

The testing code looks roughly like this:

if let Err(err) = rust_op_that_can_fail().await {
  compaction_upload_promises::resolve(&backup_id, Err(err));
  return;
}

let (future_id, future) = future_manager::new_future::<()>().await;
cpp_that_can_fail(future_id);
if let Err(err) = future.await {
  compaction_upload_promises::resolve(&backup_id, Err(err));
  return;
}

compaction_upload_promises::resolve(&backup_id, Ok(()));

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

I like the CompactionUploadPromises pattern 👍

This revision is now accepted and ready to land.Jan 29 2024, 8:03 AM

Decided to change the approach a bit:

  • instead of having a new function for notifying of compaction creation I instead created a general future manager so that we can pass Rust futures to the C++. This is based on D9925 but simplified
  • COMPACTION_UPLOAD_PROMISES introduced in the previous revision of this diff still exists as we need to wait for uploading of the compaction to the backup service before resolving the promise. It was also simplified a bit
michal requested review of this revision.Feb 7 2024, 9:30 AM
michal edited the test plan for this revision. (Show Details)
marcin added inline comments.
native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/BackupOperationsUtilities/BackupOperationsExecutor.cpp
12 ↗(On Diff #36786)

What does futureID originate from?

native/native_rust_library/src/backup/upload_handler.rs
198 ↗(On Diff #36786)

I know that this line wasn't introduced in this duff but it just caught my attention when I was taking a closer look at this diff.

What happens if compaction is 1GB of size? Are data loaded eagerly into memory at once or does tokio::fs::read handle it in a smarter lazy way?

This revision is now accepted and ready to land.Feb 8 2024, 2:00 AM
native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/BackupOperationsUtilities/BackupOperationsExecutor.cpp
12 ↗(On Diff #36786)

The futureID originates from a call to new_future in future_manager.rs. The general flow is

let (future_id, future) = future_manager::new_future::<()>().await;
// Some operation where we want to wait for the C++ to finish
create_main_compaction(&backup_id, future_id);
future.await;

You can see the real example in the next diff.

native/native_rust_library/src/backup/upload_handler.rs
198 ↗(On Diff #36786)

Good point. I don't think it handles it in a smarter way automatically, but it shouldn't be too hard to make it send compactions in batches. I will look into it.

native/native_rust_library/src/backup/upload_handler.rs
198 ↗(On Diff #36786)

Was this investigated?

native/native_rust_library/src/backup/upload_handler.rs
198 ↗(On Diff #36786)

I had this on my todo list but it wasn't directly related to this diff. It's a bit more problematic so I have created a a task for it (ENG-6976 : Streaming compaction upload) with some investigation results.