Page MenuHomePhabricator
Differential D5096

[services] Backup - Blob Get Client - Refactor put initialize
ClosedPublic
Actions

Authored by • karol on Sep 9 2022, 4:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 19, 2:24 PM
Unknown Object (File)
Tue, Mar 18, 5:21 PM
Unknown Object (File)
Tue, Mar 18, 12:08 PM
Unknown Object (File)
Sun, Mar 16, 12:39 AM
Unknown Object (File)
Sat, Mar 15, 9:13 AM
Unknown Object (File)
Thu, Mar 13, 5:46 AM
Unknown Object (File)
Wed, Mar 12, 7:22 AM
Unknown Object (File)
Tue, Mar 11, 6:06 PM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
• jon
• max
varun
tomek
Commits
rCOMMf7cf2bdf5367: [services] Backup - Blob Get Client - Refactor put initialize
Summary

Depends on D5095

Linear task: https://linear.app/comm/issue/ENG-1734/improve-error-handling-system-in-the-blob-client

Using a new way of error handling in the initialize method of the put client.

Test Plan
cd services/backup/blob_client
cargo check

after running localstack, blob and backup (in this particular order), this still works:

yarn run-performance-tests backup

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Sep 9 2022, 4:44 AM
Herald added a reviewer: • jon. · View Herald TranscriptSep 9 2022, 4:44 AM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript
• karol edited the summary of this revision. (Show Details)Sep 9 2022, 4:54 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: • max, varun, tomek.
• karol added a parent revision: D5095: [services] Backup - Blob Get Client - Refactor put read.
• karol added a child revision: D5097: [services] Backup - Blob Get Client - Cleanup.
Harbormaster completed remote builds in B12049: Diff 16548.Sep 9 2022, 4:56 AM
• karol requested review of this revision.Sep 9 2022, 4:56 AM
• jon requested changes to this revision.Sep 9 2022, 10:47 AM
• jon added inline comments.
services/backup/blob_client/src/put_client.rs
48 ↗(On Diff #16548)

functions or values named is_XXX I feel should always return a bool. Returning a result seems very odd

48–54 ↗(On Diff #16548)

Feel like the logic should be:

106 ↗(On Diff #16548)

really don't like the use of panic, as this will probably cause issues going across the C++ interface. But I think this gets handled in another diff.

But ideally, we wouldn't have an intermediate "worst case"

This revision now requires changes to proceed.Sep 9 2022, 10:47 AM
• karol added inline comments.Sep 12 2022, 1:07 AM
services/backup/blob_client/src/put_client.rs
48 ↗(On Diff #16548)

It essentially returns bool though. The "error" part of the result stands for... well, error. I don't really understand what is the problem here. The "result" syntax is just an equivalent of throw in c++ let's say.

48–54 ↗(On Diff #16548)

I don't think this is a good idea, sorry. The problem with what you proposed is that it's going to return false when we fail to lock. Failed locking should be considered an exceptional situation and handled differently than not having a client initialized.

Now, with your proposal, if this function returned false, we'd not know whether it failed to access the clients or if there's no such client in the collection.

106 ↗(On Diff #16548)

Yes, you're talking about D5097. We can do this earlier (I mean here).

• karol updated this revision to Diff 16570.Sep 12 2022, 1:16 AM

avoid panic

Harbormaster completed remote builds in B12070: Diff 16570.Sep 12 2022, 1:26 AM
• karol planned changes to this revision.Sep 12 2022, 1:35 AM
• karol updated this revision to Diff 16575.Sep 12 2022, 1:37 AM

update

Harbormaster completed remote builds in B12075: Diff 16575.Sep 12 2022, 1:48 AM
• jon accepted this revision.Sep 12 2022, 9:07 AM
This revision is now accepted and ready to land.Sep 12 2022, 9:07 AM
• karol updated this revision to Diff 16688.Sep 15 2022, 1:28 AM

rebase

Harbormaster completed remote builds in B12160: Diff 16688.Sep 15 2022, 1:38 AM
tomek accepted this revision.Sep 15 2022, 8:59 AM
tomek added inline comments.
services/backup/blob_client/src/put_client.rs
48
49–54

This is a strange pattern - it would be more readable to see this match outside. Something like the suggested code, but the case for _ may be slightly different.

64–66

ensure

115

Do we have a task / plan how we will improve this?

128

Is there a better way to handle Result than using match? Maybe anyhow can be used for that?

162

ensure

ashoat added inline comments.Sep 16 2022, 7:25 AM
services/backup/blob_client/src/put_client.rs
115

Please create task before landing and link here

• karol added inline comments.Sep 19 2022, 7:22 AM
services/backup/blob_client/src/put_client.rs
115

https://linear.app/comm/issue/ENG-1843/improve-handling-errors-in-async-stream

128

I just used ? in D5097

• karol updated this revision to Diff 16845.Sep 19 2022, 7:26 AM

rebase

This revision was landed with ongoing or failed builds.Sep 19 2022, 7:28 AM
Closed by commit rCOMMf7cf2bdf5367: [services] Backup - Blob Get Client - Refactor put initialize (authored by • karol). · Explain Why
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMMf7cf2bdf5367: [services] Backup - Blob Get Client - Refactor put initialize.
Harbormaster completed remote builds in B12283: Diff 16845.Sep 19 2022, 7:40 AM

Revision Contents
Changeset List

PathSize
services/
backup/
blob_client/
src/
10 lines
91 lines
30 lines

Diff 16688

View Options

services/backup/blob_client/src/get_client.rs

// @flow // @flow
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Text } from 'react-native';
import { oldValidUsernameRegexString } from 'lib/shared/account-utils.js'; import { oldValidUsernameRegexString } from 'lib/shared/account-utils.js';
import { import {
getNewTextAndSelection, getNewTextAndSelection,
encodeChatMentionText, encodeChatMentionText,
type Selection, type Selection,
type TypeaheadTooltipActionItem, type TypeaheadTooltipActionItem,
type MentionTypeaheadSuggestionItem, type MentionTypeaheadSuggestionItem,
} from 'lib/shared/mention-utils.js'; } from 'lib/shared/mention-utils.js';
import { validChatNameRegexString } from 'lib/shared/thread-utils.js';
import { stringForUserExplicit } from 'lib/shared/user-utils.js'; import { stringForUserExplicit } from 'lib/shared/user-utils.js';
import ThreadAvatar from '../avatars/thread-avatar.react.js';
import UserAvatar from '../avatars/user-avatar.react.js'; import UserAvatar from '../avatars/user-avatar.react.js';
import type { TypeaheadTooltipStyles } from '../chat/typeahead-tooltip.react.js'; import type { TypeaheadTooltipStyles } from '../chat/typeahead-tooltip.react.js';
// Native regex is a little bit different than web one as // Native regex is a little bit different than web one as
// there are no named capturing groups yet on native. // there are no named capturing groups yet on native.
const nativeMentionTypeaheadRegex: RegExp = new RegExp( const nativeMentionTypeaheadRegex: RegExp = new RegExp(
`((^(.|\n)*\\s+)|^)@(${oldValidUsernameRegexString})?$`, `((^(.|\n)*\\s+)|^)@(${oldValidUsernameRegexString}|${validChatNameRegexString})?$`,
); );
export type TypeaheadTooltipActionsParams<SuggestionType> = { export type TypeaheadTooltipActionsParams<SuggestionType> = {
+suggestions: $ReadOnlyArray<SuggestionType>, +suggestions: $ReadOnlyArray<SuggestionType>,
+textBeforeAtSymbol: string, +textBeforeAtSymbol: string,
+text: string, +text: string,
+textPrefix: string, +textPrefix: string,
+focusAndUpdateTextAndSelection: (text: string, selection: Selection) => void, +focusAndUpdateTextAndSelection: (text: string, selection: Selection) => void,
Show All 31 Lines if (suggestion.type === 'user') {
}); });
}, },
actionButtonContent: { actionButtonContent: {
type: 'user', type: 'user',
userInfo, userInfo,
}, },
}); });
} else if (suggestion.type === 'chat') { } else if (suggestion.type === 'chat') {
const { chat } = suggestion; const { threadInfo } = suggestion;
const mentionText = `@[[${chat.id}:${encodeChatMentionText( const mentionText = `@[[${threadInfo.id}:${encodeChatMentionText(
chat.uiName, threadInfo.uiName,
)}]]`; )}]]`;
actions.push({ actions.push({
key: chat.id, key: threadInfo.id,
execute: () => { execute: () => {
const { newText, newSelectionStart } = getNewTextAndSelection( const { newText, newSelectionStart } = getNewTextAndSelection(
textBeforeAtSymbol, textBeforeAtSymbol,
text, text,
textPrefix, textPrefix,
mentionText, mentionText,
); );
focusAndUpdateTextAndSelection(newText, { focusAndUpdateTextAndSelection(newText, {
start: newSelectionStart, start: newSelectionStart,
end: newSelectionStart, end: newSelectionStart,
}); });
}, },
actionButtonContent: { actionButtonContent: {
type: 'chat', type: 'chat',
chat, threadInfo,
}, },
}); });
} }
} }
return actions; return actions;
} }
export type TypeaheadTooltipButtonRendererParams<SuggestionItemType> = { export type TypeaheadTooltipButtonRendererParams<SuggestionItemType> = {
+item: TypeaheadTooltipActionItem<SuggestionItemType>, +item: TypeaheadTooltipActionItem<SuggestionItemType>,
+styles: TypeaheadTooltipStyles, +styles: TypeaheadTooltipStyles,
}; };
function mentionTypeaheadTooltipButtonRenderer({ function mentionTypeaheadTooltipButtonRenderer({
item, item,
styles, styles,
}: TypeaheadTooltipButtonRendererParams<MentionTypeaheadSuggestionItem>): React.Node { }: TypeaheadTooltipButtonRendererParams<MentionTypeaheadSuggestionItem>): React.Node {
let avatarComponent = null; let avatarComponent = null;
let typeaheadTooltipButtonText = null; let typeaheadTooltipButtonText = null;
if (item.actionButtonContent.type === 'user') { if (item.actionButtonContent.type === 'user') {
avatarComponent = ( avatarComponent = (
<UserAvatar size="small" userID={item.actionButtonContent.userInfo.id} /> <UserAvatar size="small" userID={item.actionButtonContent.userInfo.id} />
); );
typeaheadTooltipButtonText = item.actionButtonContent.userInfo.username; typeaheadTooltipButtonText = item.actionButtonContent.userInfo.username;
} else if (item.actionButtonContent.type === 'chat') {
typeaheadTooltipButtonText = item.actionButtonContent.threadInfo.uiName;
avatarComponent = (
<ThreadAvatar
size="small"
threadInfo={item.actionButtonContent.threadInfo}
/>
);
} }
return ( return (
<> <>
{avatarComponent} {avatarComponent}
<Text style={styles.buttonLabel} numberOfLines={1}> <Text style={styles.buttonLabel} numberOfLines={1}>
{typeaheadTooltipButtonText} {typeaheadTooltipButtonText}
</Text> </Text>
</> </>
); );
} }
export { export {
nativeMentionTypeaheadRegex, nativeMentionTypeaheadRegex,
mentionTypeaheadTooltipActions, mentionTypeaheadTooltipActions,
mentionTypeaheadTooltipButtonRenderer, mentionTypeaheadTooltipButtonRenderer,
}; };
View Options

services/backup/blob_client/src/put_client.rs

// @flow // @flow
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Text } from 'react-native';
import { oldValidUsernameRegexString } from 'lib/shared/account-utils.js'; import { oldValidUsernameRegexString } from 'lib/shared/account-utils.js';
import { import {
getNewTextAndSelection, getNewTextAndSelection,
encodeChatMentionText, encodeChatMentionText,
type Selection, type Selection,
type TypeaheadTooltipActionItem, type TypeaheadTooltipActionItem,
type MentionTypeaheadSuggestionItem, type MentionTypeaheadSuggestionItem,
} from 'lib/shared/mention-utils.js'; } from 'lib/shared/mention-utils.js';
import { validChatNameRegexString } from 'lib/shared/thread-utils.js';
import { stringForUserExplicit } from 'lib/shared/user-utils.js'; import { stringForUserExplicit } from 'lib/shared/user-utils.js';
import ThreadAvatar from '../avatars/thread-avatar.react.js';
import UserAvatar from '../avatars/user-avatar.react.js'; import UserAvatar from '../avatars/user-avatar.react.js';
import type { TypeaheadTooltipStyles } from '../chat/typeahead-tooltip.react.js'; import type { TypeaheadTooltipStyles } from '../chat/typeahead-tooltip.react.js';
// Native regex is a little bit different than web one as // Native regex is a little bit different than web one as
// there are no named capturing groups yet on native. // there are no named capturing groups yet on native.
const nativeMentionTypeaheadRegex: RegExp = new RegExp( const nativeMentionTypeaheadRegex: RegExp = new RegExp(
`((^(.|\n)*\\s+)|^)@(${oldValidUsernameRegexString})?$`, `((^(.|\n)*\\s+)|^)@(${oldValidUsernameRegexString}|${validChatNameRegexString})?$`,
); );
export type TypeaheadTooltipActionsParams<SuggestionType> = { export type TypeaheadTooltipActionsParams<SuggestionType> = {
+suggestions: $ReadOnlyArray<SuggestionType>, +suggestions: $ReadOnlyArray<SuggestionType>,
+textBeforeAtSymbol: string, +textBeforeAtSymbol: string,
+text: string, +text: string,
+textPrefix: string, +textPrefix: string,
+focusAndUpdateTextAndSelection: (text: string, selection: Selection) => void, +focusAndUpdateTextAndSelection: (text: string, selection: Selection) => void,
Show All 31 Lines if (suggestion.type === 'user') {
}); });
}, },
actionButtonContent: { actionButtonContent: {
type: 'user', type: 'user',
userInfo, userInfo,
}, },
}); });
} else if (suggestion.type === 'chat') { } else if (suggestion.type === 'chat') {
const { chat } = suggestion; const { threadInfo } = suggestion;
const mentionText = `@[[${chat.id}:${encodeChatMentionText( const mentionText = `@[[${threadInfo.id}:${encodeChatMentionText(
chat.uiName, threadInfo.uiName,
)}]]`; )}]]`;
actions.push({ actions.push({
key: chat.id, key: threadInfo.id,
execute: () => { execute: () => {
const { newText, newSelectionStart } = getNewTextAndSelection( const { newText, newSelectionStart } = getNewTextAndSelection(
textBeforeAtSymbol, textBeforeAtSymbol,
text, text,
textPrefix, textPrefix,
mentionText, mentionText,
); );
focusAndUpdateTextAndSelection(newText, { focusAndUpdateTextAndSelection(newText, {
start: newSelectionStart, start: newSelectionStart,
end: newSelectionStart, end: newSelectionStart,
}); });
}, },
actionButtonContent: { actionButtonContent: {
type: 'chat', type: 'chat',
chat, threadInfo,
}, },
}); });
} }
} }
return actions; return actions;
} }
export type TypeaheadTooltipButtonRendererParams<SuggestionItemType> = { export type TypeaheadTooltipButtonRendererParams<SuggestionItemType> = {
+item: TypeaheadTooltipActionItem<SuggestionItemType>, +item: TypeaheadTooltipActionItem<SuggestionItemType>,
+styles: TypeaheadTooltipStyles, +styles: TypeaheadTooltipStyles,
}; };
function mentionTypeaheadTooltipButtonRenderer({ function mentionTypeaheadTooltipButtonRenderer({
item, item,
styles, styles,
}: TypeaheadTooltipButtonRendererParams<MentionTypeaheadSuggestionItem>): React.Node { }: TypeaheadTooltipButtonRendererParams<MentionTypeaheadSuggestionItem>): React.Node {
let avatarComponent = null; let avatarComponent = null;
let typeaheadTooltipButtonText = null; let typeaheadTooltipButtonText = null;
if (item.actionButtonContent.type === 'user') { if (item.actionButtonContent.type === 'user') {
avatarComponent = ( avatarComponent = (
<UserAvatar size="small" userID={item.actionButtonContent.userInfo.id} /> <UserAvatar size="small" userID={item.actionButtonContent.userInfo.id} />
); );
typeaheadTooltipButtonText = item.actionButtonContent.userInfo.username; typeaheadTooltipButtonText = item.actionButtonContent.userInfo.username;
} else if (item.actionButtonContent.type === 'chat') {
typeaheadTooltipButtonText = item.actionButtonContent.threadInfo.uiName;
avatarComponent = (
<ThreadAvatar
size="small"
threadInfo={item.actionButtonContent.threadInfo}
/>
);
} }
return ( return (
<> <>
{avatarComponent} {avatarComponent}
<Text style={styles.buttonLabel} numberOfLines={1}> <Text style={styles.buttonLabel} numberOfLines={1}>
{typeaheadTooltipButtonText} {typeaheadTooltipButtonText}
</Text> </Text>
</> </>
); );
} }
export { export {
nativeMentionTypeaheadRegex, nativeMentionTypeaheadRegex,
mentionTypeaheadTooltipActions, mentionTypeaheadTooltipActions,
mentionTypeaheadTooltipButtonRenderer, mentionTypeaheadTooltipButtonRenderer,
}; };
View Options

services/backup/blob_client/src/tools.rs

// @flow // @flow
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Text } from 'react-native';
import { oldValidUsernameRegexString } from 'lib/shared/account-utils.js'; import { oldValidUsernameRegexString } from 'lib/shared/account-utils.js';
import { import {
getNewTextAndSelection, getNewTextAndSelection,
encodeChatMentionText, encodeChatMentionText,
type Selection, type Selection,
type TypeaheadTooltipActionItem, type TypeaheadTooltipActionItem,
type MentionTypeaheadSuggestionItem, type MentionTypeaheadSuggestionItem,
} from 'lib/shared/mention-utils.js'; } from 'lib/shared/mention-utils.js';
import { validChatNameRegexString } from 'lib/shared/thread-utils.js';
import { stringForUserExplicit } from 'lib/shared/user-utils.js'; import { stringForUserExplicit } from 'lib/shared/user-utils.js';
import ThreadAvatar from '../avatars/thread-avatar.react.js';
import UserAvatar from '../avatars/user-avatar.react.js'; import UserAvatar from '../avatars/user-avatar.react.js';
import type { TypeaheadTooltipStyles } from '../chat/typeahead-tooltip.react.js'; import type { TypeaheadTooltipStyles } from '../chat/typeahead-tooltip.react.js';
// Native regex is a little bit different than web one as // Native regex is a little bit different than web one as
// there are no named capturing groups yet on native. // there are no named capturing groups yet on native.
const nativeMentionTypeaheadRegex: RegExp = new RegExp( const nativeMentionTypeaheadRegex: RegExp = new RegExp(
`((^(.|\n)*\\s+)|^)@(${oldValidUsernameRegexString})?$`, `((^(.|\n)*\\s+)|^)@(${oldValidUsernameRegexString}|${validChatNameRegexString})?$`,
); );
export type TypeaheadTooltipActionsParams<SuggestionType> = { export type TypeaheadTooltipActionsParams<SuggestionType> = {
+suggestions: $ReadOnlyArray<SuggestionType>, +suggestions: $ReadOnlyArray<SuggestionType>,
+textBeforeAtSymbol: string, +textBeforeAtSymbol: string,
+text: string, +text: string,
+textPrefix: string, +textPrefix: string,
+focusAndUpdateTextAndSelection: (text: string, selection: Selection) => void, +focusAndUpdateTextAndSelection: (text: string, selection: Selection) => void,
Show All 31 Lines if (suggestion.type === 'user') {
}); });
}, },
actionButtonContent: { actionButtonContent: {
type: 'user', type: 'user',
userInfo, userInfo,
}, },
}); });
} else if (suggestion.type === 'chat') { } else if (suggestion.type === 'chat') {
const { chat } = suggestion; const { threadInfo } = suggestion;
const mentionText = `@[[${chat.id}:${encodeChatMentionText( const mentionText = `@[[${threadInfo.id}:${encodeChatMentionText(
chat.uiName, threadInfo.uiName,
)}]]`; )}]]`;
actions.push({ actions.push({
key: chat.id, key: threadInfo.id,
execute: () => { execute: () => {
const { newText, newSelectionStart } = getNewTextAndSelection( const { newText, newSelectionStart } = getNewTextAndSelection(
textBeforeAtSymbol, textBeforeAtSymbol,
text, text,
textPrefix, textPrefix,
mentionText, mentionText,
); );
focusAndUpdateTextAndSelection(newText, { focusAndUpdateTextAndSelection(newText, {
start: newSelectionStart, start: newSelectionStart,
end: newSelectionStart, end: newSelectionStart,
}); });
}, },
actionButtonContent: { actionButtonContent: {
type: 'chat', type: 'chat',
chat, threadInfo,
}, },
}); });
} }
} }
return actions; return actions;
} }
export type TypeaheadTooltipButtonRendererParams<SuggestionItemType> = { export type TypeaheadTooltipButtonRendererParams<SuggestionItemType> = {
+item: TypeaheadTooltipActionItem<SuggestionItemType>, +item: TypeaheadTooltipActionItem<SuggestionItemType>,
+styles: TypeaheadTooltipStyles, +styles: TypeaheadTooltipStyles,
}; };
function mentionTypeaheadTooltipButtonRenderer({ function mentionTypeaheadTooltipButtonRenderer({
item, item,
styles, styles,
}: TypeaheadTooltipButtonRendererParams<MentionTypeaheadSuggestionItem>): React.Node { }: TypeaheadTooltipButtonRendererParams<MentionTypeaheadSuggestionItem>): React.Node {
let avatarComponent = null; let avatarComponent = null;
let typeaheadTooltipButtonText = null; let typeaheadTooltipButtonText = null;
if (item.actionButtonContent.type === 'user') { if (item.actionButtonContent.type === 'user') {
avatarComponent = ( avatarComponent = (
<UserAvatar size="small" userID={item.actionButtonContent.userInfo.id} /> <UserAvatar size="small" userID={item.actionButtonContent.userInfo.id} />
); );
typeaheadTooltipButtonText = item.actionButtonContent.userInfo.username; typeaheadTooltipButtonText = item.actionButtonContent.userInfo.username;
} else if (item.actionButtonContent.type === 'chat') {
typeaheadTooltipButtonText = item.actionButtonContent.threadInfo.uiName;
avatarComponent = (
<ThreadAvatar
size="small"
threadInfo={item.actionButtonContent.threadInfo}
/>
);
} }
return ( return (
<> <>
{avatarComponent} {avatarComponent}
<Text style={styles.buttonLabel} numberOfLines={1}> <Text style={styles.buttonLabel} numberOfLines={1}>
{typeaheadTooltipButtonText} {typeaheadTooltipButtonText}
</Text> </Text>
</> </>
); );
} }
export { export {
nativeMentionTypeaheadRegex, nativeMentionTypeaheadRegex,
mentionTypeaheadTooltipActions, mentionTypeaheadTooltipActions,
mentionTypeaheadTooltipButtonRenderer, mentionTypeaheadTooltipButtonRenderer,
}; };