Paths

Table of Contentst

Differential D5015

[services] Backup - Blob Get Client - Add blocking read
ClosedPublic
Actions

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

Tags

None

Referenced Files

	F3342111: D5015.diff
	Fri, Nov 22, 12:35 AM

	Unknown Object (File)
	Wed, Nov 13, 1:34 AM

	Unknown Object (File)
	Tue, Nov 12, 8:46 PM

	Unknown Object (File)
	Tue, Nov 12, 10:32 AM

	Unknown Object (File)
	Sun, Nov 10, 12:18 PM

	Unknown Object (File)
	Sun, Nov 10, 6:57 AM

	Unknown Object (File)
	Sun, Nov 3, 1:44 AM

	Unknown Object (File)
	Sun, Oct 27, 8:12 PM

Subscribers

Details

Reviewers

• jon
tomek
varun
• max

Commits

rCOMM0edcf73c41af: [services] Backup - Blob Get Client - Add blocking read

Summary

Depends on D5014

Adding blocking read 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:25 AM

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

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

• karol added a parent revision: D5014: [services] Backup - Blob Get Client - Add initialize.

• karol added a child revision: D5016: [services] Backup - Blob Get Client - Add terminate.

Harbormaster completed remote builds in B11772: Diff 16175.Sep 1 2022, 4:27 AM

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

• jon requested changes to this revision.Sep 1 2022, 9:11 AM

• jon added inline comments.

services/backup/blob_client/src/get_client.rs
131–137	we pulled `client` out of the mutex guard, and now we are re-assigning it? why?
132–136	This should be a similar way to express this.

This revision now requires changes to proceed.Sep 1 2022, 9:11 AM

• karol added inline comments.Sep 5 2022, 12:06 AM

services/backup/blob_client/src/get_client.rs
131–137	Rust will not allow modifying struct members so we have to `take()` and re-put again.
132–136	Yes, looks okay, nice catch, thanks!

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

tomek added inline comments.

services/backup/blob_client/src/get_client.rs
131–137	I'm not sure if this is correct in general. For example, you can use `&mut` reference...
135	Is there a way to avoid mutating the data from outside this block? Can we use `block_on` return value for this?

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

services/backup/blob_client/src/get_client.rs
131–137	https://linear.app/comm/issue/ENG-1736/try-to-avoid-reassigning-when-modifying-the-field-of-a-global-struct
135	yes

address comments

Harbormaster completed remote builds in B11910: Diff 16353.Sep 6 2022, 2:52 AM

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

tomek added inline comments.

services/backup/blob_client/src/get_client.rs
135	Nice!

Will address concerns in https://linear.app/comm/issue/ENG-1736/try-to-avoid-reassigning-when-modifying-the-field-of-a-global-struct

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

rebase

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

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

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMM0edcf73c41af: [services] Backup - Blob Get Client - Add blocking read.

Harbormaster completed remote builds in B11992: Diff 16447.Sep 8 2022, 4:17 AM

Revision Contents
Changeset List

Path

Size

services/

backup/

blob_client/

src/

22 lines

Diff 16175

services/backup/blob_client/src/get_client.rs

Loading...