Page MenuHomePhabricator
Differential D3517

[services] Backup - Add server reactor implementations - pull backup reactor
ClosedPublic
Actions

Authored by • karol on Mar 24 2022, 1:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 26, 9:17 AM
Unknown Object (File)
Thu, Mar 13, 5:57 PM
Unknown Object (File)
Thu, Mar 13, 5:57 PM
Unknown Object (File)
Thu, Mar 13, 5:57 PM
Unknown Object (File)
Thu, Mar 13, 5:57 PM
Unknown Object (File)
Thu, Mar 13, 5:57 PM
Unknown Object (File)
Thu, Mar 13, 5:55 PM
Unknown Object (File)
Feb 23 2025, 4:45 AM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
• max
tomek
varun
• jim
Commits
rCOMMcd537913f227: [services] Backup - Add server reactor implementations - pull backup reactor
Summary

Depends on D3466

Add implementation for the pull backup reactor

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.Mar 24 2022, 1:52 AM
Herald added subscribers: • benschac, atul, • adrian and 2 others. · View Herald TranscriptMar 24 2022, 1:52 AM
Harbormaster completed remote builds in B7573: Diff 10644.Mar 24 2022, 2:11 AM
• karol requested review of this revision.Mar 24 2022, 2:11 AM
• karol edited the summary of this revision. (Show Details)Mar 24 2022, 2:19 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: • max, tomek, varun, • jim.
• karol added a parent revision: D3466: [services] Backup - Add blob client.
• karol added a child revision: D3467: [services] Backup - Add server reactor implementations - create new backup reactor.Mar 24 2022, 2:32 AM
• karol retitled this revision from [services] Backup - Add server reactor implementations - pull backup reactor to [draft] [services] Backup - Add server reactor implementations - pull backup reactor.Mar 24 2022, 2:47 AM
tomek accepted this revision.Mar 24 2022, 7:40 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
25–31 ↗(On Diff #10644)

Is there a reason why this implementation is in the header file?

This revision is now accepted and ready to land.Mar 24 2022, 7:40 AM
ashoat added inline comments.Mar 24 2022, 5:52 PM
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
25–31 ↗(On Diff #10644)

Agree that we should avoid implementations in .h files when possible. If it's addressed in a later diff, would be good to reference that here (or just keep move this stub to a .cpp file to avoid any confusion)

• karol requested review of this revision.Mar 25 2022, 7:58 AM

It is accepted as a draft so I removed the draft label and I'm requesting changes once again to keep the appropriate order of events.

services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
25–31 ↗(On Diff #10644)

Yes, I was getting weird and frustrating compile errors with templates in the background. Maybe this code can be moved to a CPP file. I don't mind as long as it works. Can we do this separately? https://linear.app/comm/issue/ENG-917/try-to-move-reactors-implementations-to-cpp-files

• karol retitled this revision from [draft] [services] Backup - Add server reactor implementations - pull backup reactor to [services] Backup - Add server reactor implementations - pull backup reactor.Mar 25 2022, 7:58 AM
• karol planned changes to this revision.Mar 28 2022, 3:43 AM
• karol updated this revision to Diff 10715.Mar 28 2022, 3:45 AM

update

Harbormaster failed remote builds in B7627: Diff 10715!Mar 28 2022, 3:51 AM
• karol updated this revision to Diff 10725.Mar 28 2022, 4:32 AM

rebuild

• karol added a comment.Mar 28 2022, 4:32 AM

ops, forgot to plan changes, sorry

Harbormaster completed remote builds in B7637: Diff 10725.Mar 28 2022, 4:40 AM
tomek accepted this revision.Mar 28 2022, 9:51 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
16

Why do we need a bidi reactor when pulling backup? Is it due to the authentication?

25–31 ↗(On Diff #10644)

Sure, we can do that

This revision is now accepted and ready to land.Mar 28 2022, 9:51 AM
• karol added inline comments.Mar 29 2022, 12:34 AM
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
16

May be an oversight. Even with auth, we needed a simple auth in this case. I think we can change this into a write reactor but maybe in a separate diff since it requires a proto file change.

• karol added inline comments.Mar 29 2022, 1:06 AM
services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h
16

https://linear.app/comm/issue/ENG-936/refactor-pullbackup-to-the-writing-reactor

Closed by commit rCOMMcd537913f227: [services] Backup - Add server reactor implementations - pull backup reactor (authored by • karol). · Explain WhyMar 31 2022, 7:21 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMMcd537913f227: [services] Backup - Add server reactor implementations - pull backup reactor.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
Reactors/
server/
35 lines

Diff 10725

View Options

services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h

#include "DatabaseManagerBase.h" #include "DatabaseManagerBase.h"
#include "Item.h" #include "Item.h"
#include <aws/core/utils/Outcome.h> #include <aws/core/utils/Outcome.h>
#include <aws/dynamodb/model/BatchWriteItemRequest.h>
#include <aws/dynamodb/model/BatchWriteItemResult.h>
#include <aws/dynamodb/model/DeleteItemRequest.h> #include <aws/dynamodb/model/DeleteItemRequest.h>
#include <iostream> #include <iostream>
namespace comm { namespace comm {
namespace network { namespace network {
namespace database { namespace database {
Show All 23 Linesvoid DatabaseManagerBase::innerRemoveItem(const Item &item) {
const Aws::DynamoDB::Model::DeleteItemOutcome &outcome = const Aws::DynamoDB::Model::DeleteItemOutcome &outcome =
getDynamoDBClient()->DeleteItem(request); getDynamoDBClient()->DeleteItem(request);
if (!outcome.IsSuccess()) { if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage()); throw std::runtime_error(outcome.GetError().GetMessage());
} }
} }
void DatabaseManagerBase::innerBatchWriteItem(
const std::string &tableName,
std::vector<Aws::DynamoDB::Model::WriteRequest> &writeRequests) {
// Split write requests to chunks by MAX_DYNAMODB_BATCH_ITEMS size and write
// them by batch
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
chunkPositionStart = writeRequests.begin() + i;
chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
tomekUnsubmitted
Done
Inline Actions
Aws::DynamoDB::Model::BatchWriteItemOutcome outcome;
- std::vector<Aws::DynamoDB::Model::WriteRequest> writeRequestsChunk;
- std::vector<Aws::DynamoDB::Model::WriteRequest>::iterator chunkPositionStart,
- chunkPositionEnd;
for (size_t i = 0; i < writeRequests.size(); i += MAX_DYNAMODB_BATCH_ITEMS) {
- chunkPositionStart = writeRequests.begin() + i;
- chunkPositionEnd = writeRequests.begin() +
+ auto chunkPositionStart = writeRequests.begin() + i;
+ auto chunkPositionEnd = writeRequests.begin() +
std::min(writeRequests.size(), i + MAX_DYNAMODB_BATCH_ITEMS);
- writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
+ auto writeRequestsChunk = std::vector<Aws::DynamoDB::Model::WriteRequest>(
chunkPositionStart, chunkPositionEnd);
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;

You can simplify the code by avoiding reusing the variables and using auto

tomek: You can simplify the code by avoiding reusing the variables and using `auto`
maxAuthorUnsubmitted
Done
Inline Actions

You can simplify the code by avoiding reusing the variables and using auto

I'm not a big fan of using auto where the type can be declared explicitly. Also, reusing the same variables is a part of loop optimization here (we don't create new ones inside the loop and reuse existing allocated memory for it).

max: > You can simplify the code by avoiding reusing the variables and using `auto` I'm not a big…
tomekUnsubmitted
Not Done
Inline Actions

Also, reusing the same variables is a part of loop optimization here

Have you measured the performance difference? I don't think this type of premature optimization is beneficial - if the variable is used only locally, it is easier to reason about it: you don't have to think about its initial value and about possible usages after the loop. Also, having a variable defined inside a loop might enable some optimization (I'm guessing now). Currently, we only reuse the name - new variables are constructed in every iteration.

So overall, we have a lot more important performance issues and saving a couple of machine instructions wouldn't be noticeable. On the other hand, from readability point of view, this is a noticeable difference - you are certain that the variable is used only where it is valid.

tomek: > Also, reusing the same variables is a part of loop optimization here Have you measured the…
Aws::DynamoDB::Model::BatchWriteItemRequest writeBatchRequest;
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
tomekUnsubmitted
Done
Inline Actions
writeBatchRequest.AddRequestItems(tableName, writeRequestsChunk);
- outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
+ auto outcome = getDynamoDBClient()->BatchWriteItem(writeBatchRequest);
if (!outcome.IsSuccess()) {
throw std::runtime_error(outcome.GetError().GetMessage());
}
}
}

outcome isn't used outside of this loop so we can declare it here

tomek: `outcome` isn't used outside of this loop so we can declare it here
maxAuthorUnsubmitted
Done
Inline Actions

outcome isn't used outside of this loop so we can declare it here

Same as above. I'm sure we should not use auto, where the type can explicitly be used, and allocating and reusing this variable here, is an optimized way from my perspective.

max: > `outcome` isn't used outside of this loop so we can declare it here Same as above. I'm sure…
}
}
} // namespace database } // namespace database
} // namespace network } // namespace network
} // namespace comm } // namespace comm