Page MenuHomePhabricator

[services][draft] Blob refactor to async API - Get
AbandonedPublic

Authored by karol on Mar 14 2022, 7:19 AM.
Tags
None
Referenced Files
F3530906: D3416.diff
Wed, Dec 25, 5:09 AM
Unknown Object (File)
Fri, Dec 20, 5:03 PM
Unknown Object (File)
Fri, Dec 20, 5:03 PM
Unknown Object (File)
Fri, Dec 20, 5:03 PM
Unknown Object (File)
Fri, Dec 20, 5:01 PM
Unknown Object (File)
Wed, Dec 4, 12:11 AM
Unknown Object (File)
Fri, Nov 29, 6:44 PM
Unknown Object (File)
Tue, Nov 26, 3:30 AM

Details

Summary

Depends on D3415

Put operation in the blob service refactored from sync to async api.

Test Plan
cd services
yarn run-blob-service

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 14 2022, 7:22 AM
Harbormaster failed remote builds in B7326: Diff 10327!
karol retitled this revision from [services] Blob - Get to [services][draft] Blob refactor to async API - Get.
karol edited the summary of this revision. (Show Details)
karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, max, varun, jim.
karol edited the summary of this revision. (Show Details)
tomek requested changes to this revision.Mar 16 2022, 10:02 AM
tomek added inline comments.
services/blob/docker-server/contents/server/src/BlobServiceImpl.cpp
163–165

Does offset > fileSize really mean success, or maybe something went wrong?

205–209

Something is wrong here. We've defined earlier

this->fileSize = getBucket(s3Path.getBucketName())
                           .getObjectSize(s3Path.getObjectName());

then we're creating a new variable

const size_t fileSize =
          bucket.getObjectSize(this->s3Path.getObjectName());

And check this->fileSize.
So the variable fileSize is never used.

It looks like we should either check this->fileSize where it is defined or check fileSize here instead

215

Is there a good reason for using a raw pointer and new here?

This revision now requires changes to proceed.Mar 16 2022, 10:02 AM

Abandoning, I'm going to put up a new stack for this which will be hopefully easier to review.