Page MenuHomePhabricator
Differential D3780

[services] Backup - Move reactors' implementations to source files
ClosedPublic
Actions

Authored by • karol on Apr 19 2022, 5:11 AM.
Tags
None
Referenced Files
F3376929: D3780.diff
Wed, Nov 27, 2:56 AM
Unknown Object (File)
Fri, Nov 22, 10:06 PM
Unknown Object (File)
Tue, Nov 19, 10:50 PM
Unknown Object (File)
Tue, Nov 19, 7:14 PM
Unknown Object (File)
Mon, Nov 18, 11:10 PM
Unknown Object (File)
Sat, Nov 16, 4:31 PM
Unknown Object (File)
Sat, Nov 16, 4:31 PM
Unknown Object (File)
Sat, Nov 16, 4:31 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• benschac
• jim
tomek

Details

Reviewers
tomek
• jim
ashoat
Commits
rCOMM5612688fda1b: [services] Backup - Move reactors' implementations to source files
Summary

Depends on D3779

https://linear.app/comm/issue/ENG-917/try-to-move-reactors-implementations-to-cpp-files

Moving reactors' implementations to the source files.

Test Plan
cd services
yarn run-backup-server

Diff Detail

Repository
rCOMM Comm
Branch
backup-blob
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Apr 19 2022, 5:11 AM
Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptApr 19 2022, 5:11 AM
• karol edited the summary of this revision. (Show Details)Apr 19 2022, 5:14 AM
• karol edited the test plan for this revision. (Show Details)
• karol added a reviewer: tomek.
• karol added a parent revision: D3779: [services] Backup - Fix: add missing pragma once to headers.
Harbormaster completed remote builds in B8305: Diff 11601.Apr 19 2022, 5:25 AM
• karol requested review of this revision.Apr 19 2022, 5:25 AM
• karol planned changes to this revision.Apr 19 2022, 7:11 AM

rebase

• karol updated this revision to Diff 11608.Apr 19 2022, 7:11 AM

rebase

Harbormaster completed remote builds in B8311: Diff 11608.Apr 19 2022, 11:00 AM
ashoat added a reviewer: • jim.Apr 19 2022, 12:15 PM
ashoat added a subscriber: • jim.

I'm confused because there is a lot of red in the diff, but not a lot of green. But the diff says it is moving stuff. Where is the stuff getting moved to? I should see some green in a .cpp file, right?

services/backup/CMakeLists.txt
39–44 ↗(On Diff #11608)

@jimpo is there a better way to do this?

• karol added a child revision: D3785: [services] Backup - Pull more mutual code from base reactors - Add base reactor.Apr 20 2022, 3:51 AM
• karol planned changes to this revision.Apr 21 2022, 3:09 AM
• karol updated this revision to Diff 11715.Apr 21 2022, 3:13 AM

rebase

Harbormaster completed remote builds in B8399: Diff 11715.Apr 21 2022, 3:24 AM
tomek requested changes to this revision.EditedApr 21 2022, 9:18 AM

Agree with @ashoat - the disproportion is concerning. Could you explain what's going on?

Edit: it's just due to how Phabricator displays this diff. When a file is split, it shows it twice:

  1. In the source, all the code that stays is white and the code that is moved is red
  2. In the destination, it shows moved code in white

The 2nd is confusing, but it happens because Phabricator "thinks" that the code was copied from somewhere, so it shows the original content in white. So every white code in .cpp should be green

This revision now requires changes to proceed.Apr 21 2022, 9:18 AM
tomek accepted this revision.Apr 21 2022, 9:27 AM

Nice! I like the fact that you spend time cleaning the imports after this refactoring

services/backup/src/Reactors/client/blob/BlobAppendHolderClientReactor.h
10 ↗(On Diff #11715)

Why do we need to add an import? The amount of code decreased, so the number of imports shouldn't grow, right?

This revision now requires review to proceed.Apr 21 2022, 9:27 AM
• karol planned changes to this revision.Apr 22 2022, 3:46 AM

rebase in progress

• karol updated this revision to Diff 11785.Apr 22 2022, 3:48 AM

rebase

Harbormaster completed remote builds in B8446: Diff 11785.Apr 22 2022, 4:04 AM
• karol planned changes to this revision.Apr 22 2022, 5:35 AM
In D3780#105381, @palys-swm wrote:

Agree with @ashoat - the disproportion is concerning. Could you explain what's going on?

Edit: it's just due to how Phabricator displays this diff. When a file is split, it shows it twice:

  1. In the source, all the code that stays is white and the code that is moved is red
  2. In the destination, it shows moved code in white

The 2nd is confusing, but it happens because Phabricator "thinks" that the code was copied from somewhere, so it shows the original content in white. So every white code in .cpp should be green

Thanks for clarifying!

Another rebase in progress

• karol updated this revision to Diff 11796.Apr 22 2022, 5:45 AM

rebase

Harbormaster completed remote builds in B8457: Diff 11796.Apr 22 2022, 5:50 AM
ashoat added a comment.Apr 24 2022, 4:29 PM

Highlighting my question for @jimpo again (@karol-bisztyga feel free to ping him if no response)

services/backup/CMakeLists.txt
39–44 ↗(On Diff #11796)

@jimpo is there a better way to do this?

• karol planned changes to this revision.Apr 25 2022, 12:32 AM
• karol added inline comments.
services/backup/CMakeLists.txt
39–44 ↗(On Diff #11796)

we can use GLOB_RECURSE

• karol updated this revision to Diff 11837.Apr 25 2022, 12:33 AM

use GLOB_RECURSE

Harbormaster completed remote builds in B8491: Diff 11837.Apr 25 2022, 12:38 AM
ashoat accepted this revision.Apr 25 2022, 9:46 AM
ashoat added a reviewer: ashoat. ashoat removed 1 blocking reviewer(s): • jim.

Awesome

This revision is now accepted and ready to land.Apr 25 2022, 9:46 AM
Closed by commit rCOMM5612688fda1b: [services] Backup - Move reactors' implementations to source files (authored by • karol). · Explain WhyApr 27 2022, 2:38 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM5612688fda1b: [services] Backup - Move reactors' implementations to source files.

Revision Contents
Changeset List

PathSize
services/
backup/
7 lines
src/
Reactors/
client/
blob/
24 lines
32 lines
26 lines
BlobGetClientReactor.cpp
BlobGetClientReactor.h
28 lines
60 lines
BlobPutClientReactor.cpp
BlobPutClientReactor.h
47 lines
server/
118 lines
CreateNewBackupReactor.cpp
CreateNewBackupReactor.h
50 lines
123 lines
PullBackupReactor.cpp
PullBackupReactor.h
45 lines
178 lines
SendLogReactor.cpp
SendLogReactor.h
66 lines

Diff 11601

View Options

services/backup/CMakeLists.txt

Loading...
View Options

services/backup/src/Reactors/client/blob/BlobAppendHolderClientReactor.h

Loading...
View Options

services/backup/src/Reactors/client/blob/BlobAppendHolderClientReactor.cpp

Loading...
View Options

services/backup/src/Reactors/client/blob/BlobGetClientReactor.h

Loading...
View Options

services/backup/src/Reactors/client/blob/BlobGetClientReactor.cpp

Loading...
View Options

services/backup/src/Reactors/client/blob/BlobPutClientReactor.h

Loading...
View Options

services/backup/src/Reactors/client/blob/BlobPutClientReactor.cpp

Loading...
View Options

services/backup/src/Reactors/server/CreateNewBackupReactor.h

Loading...
View Options

services/backup/src/Reactors/server/CreateNewBackupReactor.cpp

Loading...
View Options

services/backup/src/Reactors/server/PullBackupReactor.h

Loading...
View Options

services/backup/src/Reactors/server/PullBackupReactor.cpp

Loading...
View Options

services/backup/src/Reactors/server/SendLogReactor.h

Loading...
View Options

services/backup/src/Reactors/server/SendLogReactor.cpp

Loading...