Page MenuHomePhabricator

Implement methods to start, continue and resume gRPC blob streaming
AbandonedPublic

Authored by marcin on Sep 5 2022, 8:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 10:39 AM
Unknown Object (File)
Mon, Nov 25, 6:20 PM
Unknown Object (File)
Mon, Nov 25, 6:15 PM
Unknown Object (File)
Mon, Nov 25, 1:44 AM
Unknown Object (File)
Thu, Nov 14, 9:48 AM
Unknown Object (File)
Tue, Nov 12, 3:51 PM
Unknown Object (File)
Tue, Nov 12, 3:29 AM
Unknown Object (File)
Sat, Nov 9, 3:38 PM

Details

Summary

This differential implements methods to start, continue and resume multimedia upload process in rust using gRPC in a way suitable for mobile devices

Test Plan

Start blob service locally. Place the following code at the end of lib.rs:

#[tokio::test]
async fn upload_test() {
  let holder = "test".to_string();
  let hash = "test".to_string();
  let upload_data = vec!["test".to_string(), "test".to_string()];

  let mut upload_state = initialize_upload_state().await.unwrap();

  start_upload(&mut upload_state, holder, hash).await.unwrap();
  for chunk in upload_data.iter() {
    upload_chunk(&mut upload_state, &chunk).await.unwrap();
  }
  resume_upload(upload_state).await.unwrap();
}

Test should pass. Then run:

aws --endpoint-url=http://localhost:4566 s3 cp s3://commapp-blob/test localfile
cat localfile

You should see 'testtest` as localfile content

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin requested review of this revision.Sep 5 2022, 8:40 AM
tomek added a reviewer: karol.
tomek added inline comments.
native/cpp/CommonCpp/grpc/blob_client/rust/src/lib.rs
69 ↗(On Diff #16332)

Why do we need to wrap it with Box?

72 ↗(On Diff #16332)

Shouldn't we use a kind of error instead of just strings?

104 ↗(On Diff #16332)

I'm not sure if this is the right name. We're ending the process here instead of resuming it.

native/cpp/CommonCpp/grpc/blob_client/rust/src/lib.rs
69 ↗(On Diff #16332)

In parent differential it is explained why we need to return a Box to C++ instead of the variable directly. Passing Box here is a consequence. This function is expected to be called from C++. Since C++ gets UploadState in a Box we need to pass Box here as well.

72 ↗(On Diff #16332)

This function is also designed to be exposed to C++ by cxx bridge. We could use some error here but we would not have any benefits of doing so. According to cxx docs: https://cxx.rs/binding/result.html every rust error is transferred to C++ as std::exception which e.what() will return string returned by calling Display on the corresponding rust error. In summary, we can use some custom errors but the effect will be the same. This code is currently expected to be called exclusively from C++. If we ever decide to share it with another Rust code we will probably have to implement some error structure.

104 ↗(On Diff #16332)

You are right. That is definitely a misused word.

Rename resume to complete

This revision is now accepted and ready to land.Sep 8 2022, 4:27 AM

Rust String requires that it si UTF-8 correct which is not guaranteed by bytes in C++ (char* unit8_t *, std::string). That said we will send &[u8] instead.

await on send on channel since it is bounded now

tomek added inline comments.
native/cpp/CommonCpp/grpc/blob_client/rust/src/lib.rs
97–102 ↗(On Diff #16493)

Can we return the result of map_err directly?

113 ↗(On Diff #16493)

What this doble question mark does? Can we somehow return the result of map_err instead of using question mark and returning Ok explicitly?

native/cpp/CommonCpp/grpc/blob_client/rust/src/lib.rs
97–102 ↗(On Diff #16493)

Yes, you are right. We can do it.

113 ↗(On Diff #16493)

According to tokio docs (https://docs.rs/tokio/latest/tokio/task/struct.JoinHandle.html) awaiting on JoinHandle gives you value returned from task wrapped inside Result<>. Since value returned from receiver_task is Result<> itself we need to double ? to get to the actual value. In other words: we double ? since the value we are interested in is wrapped in two nested Result<>. But we can return result of map_err directly by changing:

.map_err(|e| format!("Error occurred on consumer task. Details {}", e))??;
Ok(result)

to:

.map_err(|e| format!("Error occurred on consumer task. Details {}", e))?

Just note that for the reason explained above we still need one ?.

Refactor after varun's changes

Diff from 2022 what I was assigned encrypted blob upload work.

Diff from 2022 what I was assigned encrypted blob upload work.