Page MenuHomePhabricator
Differential D3774

[services] Backup - Add append holder reactor
AbandonedPublic
Actions

Authored by • karol on Apr 19 2022, 1:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 12:43 AM
Unknown Object (File)
Sat, Nov 16, 4:29 PM
Unknown Object (File)
Mon, Nov 11, 5:47 AM
Unknown Object (File)
Tue, Nov 5, 5:10 PM
Unknown Object (File)
Oct 15 2024, 6:29 PM
Unknown Object (File)
Oct 13 2024, 3:06 PM
Unknown Object (File)
Oct 13 2024, 3:06 PM
Unknown Object (File)
Oct 13 2024, 2:58 PM
View All 100 Files
Subscribers
• abosh
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
tomek
ashoat
Summary

Depends on D3773

Adding BlobAppendHolderClientReactor to the backup service

https://linear.app/comm/issue/ENG-672/create-addreferencetohashbyholder-in-the-blob-service

Test Plan
cd services
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Apr 19 2022, 1:56 AM
Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptApr 19 2022, 1:56 AM
• karol edited the summary of this revision. (Show Details)Apr 19 2022, 2:17 AM
• karol edited the test plan for this revision. (Show Details)
• karol added a reviewer: tomek.
• karol added a parent revision: D3773: [services] Blob - Add append holder implementation.
• karol added a child revision: D3775: [services] Backup - Add data exists flag to put client reactor.
Harbormaster completed remote builds in B8296: Diff 11592.Apr 19 2022, 2:22 AM
• karol requested review of this revision.Apr 19 2022, 2:22 AM
ashoat added a comment.Apr 19 2022, 10:50 AM

I suspect it probably would've been best to hold off on putting these diffs up... it feels like you just made three commits and diffed them immediately, but I suspect we will see a lot more commits you make to these files and subsequently a lot more diffs...

A good way to think about it: make sure you have a way to test something before you put up a diff up. In this case, looking at your test plans it seems like you don't have anything other than simply compiling the service.

tomek requested changes to this revision.Apr 21 2022, 2:12 AM

Is there a way to create a test plan that verifies the logic?

This revision now requires changes to proceed.Apr 21 2022, 2:12 AM
• karol requested review of this revision.Apr 21 2022, 6:56 AM

Kind of hard to come up with a different test plan than a successful compilation in a case when this code's not used anywhere. This is used in D3777 where the test plan points to D3535 and covers the client-server communication.

tomek accepted this revision.Apr 21 2022, 8:37 AM
In D3774#105264, @karol-bisztyga wrote:

Kind of hard to come up with a different test plan than a successful compilation in a case when this code's not used anywhere. This is used in D3777 where the test plan points to D3535 and covers the client-server communication.

Ok, that makes sense. I find it useful to sometimes write new code just for the testing purpose - that might help. This code is usually really ugly, so it's costly to transform it into proper tests, but it lets me verify the correctness quickly.

This revision is now accepted and ready to land.Apr 21 2022, 8:37 AM
ashoat requested changes to this revision.Apr 21 2022, 3:16 PM

See feedback in D3772

This revision now requires changes to proceed.Apr 21 2022, 3:16 PM
• karol abandoned this revision.Apr 22 2022, 3:27 AM

This is a part of mistakenly added code, more details in https://phabricator.ashoat.com/D3772#105661.

Abandoning. Sorry for the inconvenience, thank you for the review.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
Reactors/
client/
blob/
51 lines

Diff 11592

View Options

services/backup/docker-server/contents/server/src/Reactors/client/blob/BlobAppendHolderClientReactor.h

Loading...