Paths

Table of Contentst

Differential D3651

[services] Backup - Send log - finish read request logic
ClosedPublic
Actions

Authored by • karol on Apr 7 2022, 7:22 AM.

Tags

None

Referenced Files

	Unknown Object (File)
	Sun, Jun 30, 12:17 PM

	Unknown Object (File)
	Sun, Jun 30, 11:13 AM

	Unknown Object (File)
	Sun, Jun 30, 11:13 AM

	Unknown Object (File)
	Sun, Jun 30, 11:13 AM

	Unknown Object (File)
	Sun, Jun 30, 11:13 AM

	Unknown Object (File)
	Sun, Jun 30, 11:10 AM

	Unknown Object (File)
	Wed, Jun 26, 5:40 PM

	Unknown Object (File)
	Wed, Jun 26, 4:12 PM

Subscribers

Details

Reviewers

tomek
• max

Commits

rCOMMc703b8a03b42: [services] Backup - Send log - finish read request logic

Summary

Depends on D3650

Adding logic for certain state options in order to properly handle sending logs to the backup service.

Test Plan

You need 3 terminals
1:

cd services
yarn run-backup-service

2:

cd services
yarn run-blob-service

3:
You can use https://github.com/karol-bisztyga/grpc-playground/tree/backup-async

./build.sh
./cmake/build/bin/client

then in the GUI use the n option (just type n and hit enter, this stands for creating a new backup).

Diff Detail

Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• karol created this revision.Apr 7 2022, 7:22 AM

Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptApr 7 2022, 7:22 AM

• karol added a child revision: D3594: [services] Blob - Fix reading data from S3.Apr 7 2022, 7:23 AM

• karol edited the summary of this revision. (Show Details)Apr 7 2022, 7:33 AM

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: tomek, • max.

• karol added a parent revision: D3650: [services] Backup - Send log - add terminate callback.

• karol mentioned this in D3586: [services] Backup - Send log - add store in database.Apr 7 2022, 7:36 AM

Harbormaster completed remote builds in B7971: Diff 11175.Apr 7 2022, 8:41 AM

• karol requested review of this revision.Apr 7 2022, 8:41 AM

tomek accepted this revision.Apr 8 2022, 1:56 AM

tomek added inline comments.

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗	(On Diff #11175)	I'm wondering if it was a good idea to store either log or holder in value. Maybe the code would be more readable if we had two separate fields. What do you think?

This revision is now accepted and ready to land.Apr 8 2022, 1:56 AM

rebase

Harbormaster completed remote builds in B8015: Diff 11220.Apr 8 2022, 2:55 AM

• karol mentioned this in D3647: [services] Backup - Send log - add backup id and log hash states.Apr 13 2022, 4:19 AM

• karol added inline comments.Apr 13 2022, 4:28 AM

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗	(On Diff #11175)	Well, maybe, Basically, this was agreed on during the arch review. Maybe you're right. If you feel like it, please create a task and assign it to me, we can discuss if it is a good change.

Closed by commit rCOMMc703b8a03b42: [services] Backup - Send log - finish read request logic (authored by • karol). · Explain WhyApr 13 2022, 4:58 AM

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMMc703b8a03b42: [services] Backup - Send log - finish read request logic.

tomek added inline comments.Apr 13 2022, 8:05 AM

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗	(On Diff #11175)	It was just an idea. If you think it would simplify things, we can do this as a followup. If you're not convinced, then creating a task doesn't make a lot of sense. So up to you. But overall, you've written this code, so do you think that having separate fields would simplify the implementation?

ashoat added inline comments.Apr 18 2022, 11:42 AM

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗	(On Diff #11175)	Basically, this was agreed on during the arch review It's true we agreed on using the same database field for these two values, but that doesn't mean we need to have the same variable for it in `SendLogReactor`. I agree with @palys-swm that it would be more readable to use different variables for these

• karol added inline comments.May 6 2022, 4:56 AM

services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h
163 ↗	(On Diff #11175)	https://linear.app/comm/issue/ENG-1092/store-holder-and-value-as-2-separate-fields-in-backup

Revision Contents
Changeset List

Path

Size

services/

backup/

docker-server/

contents/

server/

src/

Reactors/

server/

SendLogReactor.h

41 lines

Diff 11399

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

Loading...