Paths

Table of Contentst

Differential D5014

[services] Backup - Blob Get Client - Add initialize
ClosedPublic
Actions

Authored by • karol on Sep 1 2022, 4:09 AM.

Tags

None

Referenced Files

	Unknown Object (File)
	Fri, Dec 20, 12:53 AM

	Unknown Object (File)
	Sat, Dec 14, 6:15 AM

	Unknown Object (File)
	Sat, Dec 14, 6:15 AM

	Unknown Object (File)
	Sat, Dec 14, 6:15 AM

	Unknown Object (File)
	Sat, Dec 14, 6:15 AM

	Unknown Object (File)
	Sat, Dec 14, 6:14 AM

	Unknown Object (File)
	Sat, Dec 14, 6:08 AM

	Unknown Object (File)
	Fri, Dec 13, 11:03 PM

Subscribers

Details

Reviewers

• jon
tomek
varun
• max

Commits

rCOMM8949d823fda9: [services] Backup - Blob Get Client - Add initialize

Summary

Depends on D5013

Adding initialize function to the blob get client.

Test Plan

backup builds

Diff Detail

Repository

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Sep 1 2022, 4:09 AM

Herald added a reviewer: • jon. · View Herald TranscriptSep 1 2022, 4:09 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

• karol edited the summary of this revision. (Show Details)Sep 1 2022, 4:24 AM

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

• karol added reviewers: tomek, varun, • max.

• karol added a parent revision: D5013: [services] Backup - Blob Get Client - Add base structure.

• karol added a child revision: D5015: [services] Backup - Blob Get Client - Add blocking read.

Harbormaster completed remote builds in B11771: Diff 16174.Sep 1 2022, 4:25 AM

• karol requested review of this revision.Sep 1 2022, 4:25 AM

tomek requested changes to this revision.Sep 5 2022, 5:33 AM

tomek added inline comments.

services/backup/blob_client/src/get_client.rs
64–69 ↗	(On Diff #16174)	This feels unsafe. What should be the result of `is_initialized()` after `get_client_terminate_cxx`? Can we simplify the logic, so that handling repeated initializations is contained within the `if` and we exit the function there?
86–88 ↗	(On Diff #16174)	We can simplify the code by using `loop` and `break`

This revision now requires changes to proceed.Sep 5 2022, 5:33 AM

address comments

• karol added inline comments.Sep 6 2022, 2:06 AM

services/backup/blob_client/src/get_client.rs
64–69 ↗	(On Diff #16174)	This feels unsafe. What should be the result of `is_initialized()` after `get_client_terminate_cxx`? `false` Can we simplify the logic, so that handling repeated initializations is contained within the `if` and we exit the function there? I wouldn't suppress such an action. Re-initialization was introduced in order to simplify the code in c++, so we don't have to call `terminate` explicitly every time. This function (`initialize`) therefore stands for initialization/re-initialization. I don't know, I feel pretty good about this, could you specify what exactly is unsafe here? Or any "real" arguments against the current solution?
86–88 ↗	(On Diff #16174)	yes, right

Harbormaster completed remote builds in B11909: Diff 16352.Sep 6 2022, 2:14 AM

tomek accepted this revision.Sep 7 2022, 5:11 AM

tomek added inline comments.

services/backup/blob_client/src/get_client.rs
85–102		Maybe we can simplify this even further by using `while let` which would match against data from response?
64–69 ↗	(On Diff #16174)	Ok, that makes sense.

This revision is now accepted and ready to land.Sep 7 2022, 5:11 AM

• karol added inline comments.Sep 8 2022, 3:41 AM

services/backup/blob_client/src/get_client.rs
85–102	That is ok but we'd not have error message available when using `while let`

rebase

This revision was landed with ongoing or failed builds.Sep 8 2022, 4:10 AM

Closed by commit rCOMM8949d823fda9: [services] Backup - Blob Get Client - Add initialize (authored by • karol). · Explain Why

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMM8949d823fda9: [services] Backup - Blob Get Client - Add initialize.

Harbormaster completed remote builds in B11991: Diff 16446.Sep 8 2022, 4:17 AM

Revision Contents
Changeset List

Path

Size

services/

backup/

blob_client/

src/

61 lines

Diff 16352

services/backup/blob_client/src/get_client.rs

Loading...