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	@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

rebase

Harbormaster completed remote builds in B8399: Diff 11715.Apr 21 2022, 3:24 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:

In the source, all the code that stays is white and the code that is moved is red
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

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

rebase in progress

rebase

Harbormaster completed remote builds in B8446: Diff 11785.Apr 22 2022, 4:04 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:

In the source, all the code that stays is white and the code that is moved is red

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

rebase

Harbormaster completed remote builds in B8457: Diff 11796.Apr 22 2022, 5:50 AM

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`

use GLOB_RECURSE

Harbormaster completed remote builds in B8491: Diff 11837.Apr 25 2022, 12:38 AM

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

Path

Size

services/

backup/

CMakeLists.txt

7 lines

src/

Reactors/

client/

blob/

BlobAppendHolderClientReactor.h

24 lines

BlobAppendHolderClientReactor.cpp

32 lines

BlobGetClientReactor.h

26 lines

	BlobGetClientReactor.cpp
	BlobGetClientReactor.h

28 lines

BlobPutClientReactor.h

60 lines

	BlobPutClientReactor.cpp
	BlobPutClientReactor.h

47 lines

server/

CreateNewBackupReactor.h

118 lines

	CreateNewBackupReactor.cpp
	CreateNewBackupReactor.h

50 lines

PullBackupReactor.h

117 lines

	PullBackupReactor.cpp
	PullBackupReactor.h

110 lines

SendLogReactor.h

178 lines

	SendLogReactor.cpp
	SendLogReactor.h

66 lines

Diff 11608

View Options

services/backup/CMakeLists.txt

Show First 20 Lines • Show All 69 Lines • ▼ Show 20 Lines	switch (this->state) {
case State::DATA_CHUNKS: {		case State::DATA_CHUNKS: {
// TODO initialize blob client reactor		// TODO initialize blob client reactor
if (ServiceBlobClient::getInstance().putReactor == nullptr) {		if (ServiceBlobClient::getInstance().putReactor == nullptr) {
throw std::runtime_error(		throw std::runtime_error(
"blob client reactor has not been initialized");		"blob client reactor has not been initialized");
}		}

ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(		ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(
request.newcompactionchunk());		std::make_unique<std::string>(
		std::move(*request.mutable_newcompactionchunk())));

return nullptr;		return nullptr;
}		}
}		}
throw std::runtime_error("new backup - invalid state");		throw std::runtime_error("new backup - invalid state");
}		}

void CreateNewBackupReactor::doneCallback() {		void CreateNewBackupReactor::doneCallback() {
ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk("");		ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(
		std::make_unique<std::string>(""));
}		}

} // namespace reactor		} // namespace reactor
} // namespace network		} // namespace network
} // namespace comm		} // namespace comm

View Options

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

Show First 20 Lines • Show All 69 Lines • ▼ Show 20 Lines	switch (this->state) {
case State::DATA_CHUNKS: {		case State::DATA_CHUNKS: {
// TODO initialize blob client reactor		// TODO initialize blob client reactor
if (ServiceBlobClient::getInstance().putReactor == nullptr) {		if (ServiceBlobClient::getInstance().putReactor == nullptr) {
throw std::runtime_error(		throw std::runtime_error(
"blob client reactor has not been initialized");		"blob client reactor has not been initialized");
}		}

ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(		ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(
request.newcompactionchunk());		std::make_unique<std::string>(
		std::move(*request.mutable_newcompactionchunk())));

return nullptr;		return nullptr;
}		}
}		}
throw std::runtime_error("new backup - invalid state");		throw std::runtime_error("new backup - invalid state");
}		}

void CreateNewBackupReactor::doneCallback() {		void CreateNewBackupReactor::doneCallback() {
ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk("");		ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(
		std::make_unique<std::string>(""));
}		}

} // namespace reactor		} // namespace reactor
} // namespace network		} // namespace network
} // namespace comm		} // namespace comm

View Options

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

Show First 20 Lines • Show All 69 Lines • ▼ Show 20 Lines	switch (this->state) {
case State::DATA_CHUNKS: {		case State::DATA_CHUNKS: {
// TODO initialize blob client reactor		// TODO initialize blob client reactor
if (ServiceBlobClient::getInstance().putReactor == nullptr) {		if (ServiceBlobClient::getInstance().putReactor == nullptr) {
throw std::runtime_error(		throw std::runtime_error(
"blob client reactor has not been initialized");		"blob client reactor has not been initialized");
}		}

ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(		ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(
request.newcompactionchunk());		std::make_unique<std::string>(
		std::move(*request.mutable_newcompactionchunk())));

return nullptr;		return nullptr;
}		}
}		}
throw std::runtime_error("new backup - invalid state");		throw std::runtime_error("new backup - invalid state");
}		}

void CreateNewBackupReactor::doneCallback() {		void CreateNewBackupReactor::doneCallback() {
ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk("");		ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(
		std::make_unique<std::string>(""));
}		}

} // namespace reactor		} // namespace reactor
} // namespace network		} // namespace network
} // namespace comm		} // namespace comm

View Options

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

Show First 20 Lines • Show All 69 Lines • ▼ Show 20 Lines	switch (this->state) {
case State::DATA_CHUNKS: {		case State::DATA_CHUNKS: {
// TODO initialize blob client reactor		// TODO initialize blob client reactor
if (ServiceBlobClient::getInstance().putReactor == nullptr) {		if (ServiceBlobClient::getInstance().putReactor == nullptr) {
throw std::runtime_error(		throw std::runtime_error(
"blob client reactor has not been initialized");		"blob client reactor has not been initialized");
}		}

ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(		ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(
request.newcompactionchunk());		std::make_unique<std::string>(
		std::move(*request.mutable_newcompactionchunk())));

return nullptr;		return nullptr;
}		}
}		}
throw std::runtime_error("new backup - invalid state");		throw std::runtime_error("new backup - invalid state");
}		}

void CreateNewBackupReactor::doneCallback() {		void CreateNewBackupReactor::doneCallback() {
ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk("");		ServiceBlobClient::getInstance().putReactor->scheduleSendingDataChunk(
		std::make_unique<std::string>(""));
}		}

} // namespace reactor		} // namespace reactor
} // namespace network		} // namespace network
} // namespace comm		} // namespace comm