Page MenuHomePhabricator
Differential D5030

[services] Rust Integration - Backup - Make report_error shared
ClosedPublic
Actions

Authored by • karol on Sep 1 2022, 11:54 PM.
Tags
None
Referenced Files
F5113872: D5030.id16485.diff
Wed, Apr 2, 6:18 AM
F5109794: D5030.diff
Tue, Apr 1, 8:50 PM
Unknown Object (File)
Sun, Mar 30, 7:32 AM
Unknown Object (File)
Sun, Mar 30, 7:32 AM
Unknown Object (File)
Sun, Mar 30, 7:32 AM
Unknown Object (File)
Sun, Mar 30, 7:32 AM
Unknown Object (File)
Sun, Mar 30, 7:32 AM
Unknown Object (File)
Sun, Mar 30, 7:32 AM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
• jon
tomek
• max
varun
Commits
rCOMM8236a5fbe0a4: [services] Rust Integration - Backup - Make report_error shared
Summary

Depends on D5029

I just moved the function report_error to the mutual space tools following the DRY rule.

Test Plan
cd services
yarn run-integration-tests backup

Works as before

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Sep 1 2022, 11:54 PM
Herald added a reviewer: • jon. · View Herald TranscriptSep 1 2022, 11:54 PM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript
• karol updated this revision to Diff 16226.Sep 2 2022, 12:02 AM

remove comment

• karol edited the summary of this revision. (Show Details)Sep 2 2022, 12:04 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: tomek, • max, varun.
• karol edited the summary of this revision. (Show Details)
• karol added a parent revision: D5029: [services] Rust Integration - Backup - Stop rethrowing in create new backup reactor.
Harbormaster completed remote builds in B11818: Diff 16223.Sep 2 2022, 12:05 AM
• karol added a child revision: D5031: [services] Rust Integration - Backup - Make check_error shared.Sep 2 2022, 12:05 AM
Harbormaster completed remote builds in B11821: Diff 16226.Sep 2 2022, 12:10 AM
• karol requested review of this revision.Sep 2 2022, 12:10 AM
• karol planned changes to this revision.Sep 2 2022, 12:34 AM
• karol updated this revision to Diff 16228.Sep 2 2022, 12:35 AM

update

• karol planned changes to this revision.Sep 2 2022, 12:35 AM
• karol updated this revision to Diff 16230.Sep 2 2022, 12:37 AM

update

Harbormaster failed remote builds in B11825: Diff 16230!Sep 2 2022, 12:40 AM
Harbormaster completed remote builds in B11823: Diff 16228.Sep 2 2022, 12:44 AM
• karol planned changes to this revision.Sep 2 2022, 12:54 AM

CI

• karol updated this revision to Diff 16239.Sep 2 2022, 4:15 AM

update

Harbormaster completed remote builds in B11834: Diff 16239.Sep 2 2022, 4:23 AM
• karol mentioned this in D5011: [services] Backup - Blob Put Client - Add write.Sep 2 2022, 7:06 AM
• jon requested changes to this revision.Sep 2 2022, 11:11 AM
• jon added inline comments.
services/backup/blob_client/src/tools.rs
5 ↗(On Diff #16239)

Why pass a global variable? It's always going to be available.

Taking a step back, we shouldn't be using global variables; if we just want to see if something is in a valid state, we should attach the state to the respective resource (e.g. client).

9–12 ↗(On Diff #16239)

what's the value that labels provide? Couldn't the message just contain it.

If we were using log statements, one could also pass a target argument.

We would just need to initiate the logger alongside the runtime.

https://docs.rs/log/latest/log/macro.info.html

This revision now requires changes to proceed.Sep 2 2022, 11:11 AM
• karol mentioned this in D5013: [services] Backup - Blob Get Client - Add base structure.Sep 6 2022, 1:43 AM
• karol requested review of this revision.Sep 6 2022, 3:54 AM
• karol added inline comments.
services/backup/blob_client/src/tools.rs
5 ↗(On Diff #16239)

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

9–12 ↗(On Diff #16239)

I think we can do this in this task https://linear.app/comm/issue/ENG-1717/use-tracing-utilities-instead-of-println

I'm going to add a note.

• karol planned changes to this revision.Sep 6 2022, 7:17 AM
• karol updated this revision to Diff 16369.Sep 6 2022, 7:17 AM

update

Harbormaster completed remote builds in B11923: Diff 16369.Sep 6 2022, 7:27 AM
• jon added inline comments.Sep 6 2022, 9:55 AM
services/backup/blob_client/src/tools.rs
5 ↗(On Diff #16239)

I think this is kind of side stepping the issue.

I think the current complexity could be reduced by just emitting the errors and representing the check_errors() as checking to see if a Mutex<bool> is true.

Even better would be marking the individual clients as healthy and have some logic to get them back to healthy, but that's more work, and should be done later as another task.

• jon accepted this revision.Sep 7 2022, 11:18 AM

https://linear.app/comm/issue/ENG-1717/use-tracing-utilities-instead-of-println

This revision is now accepted and ready to land.Sep 7 2022, 11:18 AM
• karol updated this revision to Diff 16458.Sep 8 2022, 4:03 AM

rebase

This revision was landed with ongoing or failed builds.Sep 8 2022, 4:10 AM
Closed by commit rCOMM8236a5fbe0a4: [services] Rust Integration - Backup - Make report_error shared (authored by • karol). · Explain Why
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM8236a5fbe0a4: [services] Rust Integration - Backup - Make report_error shared.
Harbormaster completed remote builds in B12003: Diff 16458.Sep 8 2022, 4:32 AM

Revision Contents
Changeset List

PathSize
services/
backup/
blob_client/
src/
36 lines
1 line
66 lines
18 lines

Diff 16458

View Options

services/backup/blob_client/src/get_client.rs

// @flow // @flow
import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements'; import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Alert, Text } from 'react-native';
import { TouchableOpacity } from 'react-native-gesture-handler'; import { TouchableOpacity } from 'react-native-gesture-handler';
import type { NavigationRoute } from '../navigation/route-names';
import { useColors } from '../themes/colors.js'; import { useColors } from '../themes/colors.js';
type Props = React.ElementConfig<typeof BaseHeaderBackButton>; type ChangeRolesHeaderLeftButtonProps = {
function ChangeRolesHeaderLeftButton(props: Props): React.Node { +route: NavigationRoute<'ChangeRolesScreen'>,
...React.ElementConfig<typeof BaseHeaderBackButton>,
};
function ChangeRolesHeaderLeftButton(
props: ChangeRolesHeaderLeftButtonProps,
): React.Node {
const { memberInfo, role: selectedRole } = props.route.params;
const { role: memberRole } = memberInfo;
const onCancel = React.useCallback(() => {
const { onPress } = props;
invariant(onPress, 'onPress must be defined');
if (selectedRole === memberRole) {
onPress();
return;
}
Alert.alert(
'Unsaved Changes',
'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
[
{ text: 'Cancel', onPress },
{ text: 'Stay', style: 'cancel' },
],
{ cancelable: true },
);
}, [memberRole, props, selectedRole]);
rohanAuthorUnsubmitted
Done
Inline Actions

I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes.

Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted)

rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
ashoatUnsubmitted
Not Done
Inline Actions

What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted

ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
rohanAuthorUnsubmitted
Done
Inline Actions

Ah my bad. It looks like in message editing: Continue editing closes the alert and continues editing while Discard edit closes the alert and exits editing

rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
ashoatUnsubmitted
Not Done
Inline Actions
  1. I agree we shouldn't use the term Cancel here... how about Leave?
  2. I don't think we need { cancelable: true }... we can just default to Stay if the user dismisses the Alert
  3. Can we try to match copy for title / description with the edit-message workflow?
ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah that makes sense, I'll update the diff

rohan: Yeah that makes sense, I'll update the diff
const { panelForegroundSecondaryLabel } = useColors(); const { panelForegroundSecondaryLabel } = useColors();
const labelStyle = React.useMemo( const labelStyle = React.useMemo(
() => ({ () => ({
color: panelForegroundSecondaryLabel, color: panelForegroundSecondaryLabel,
marginLeft: 10, marginLeft: 10,
}), }),
[panelForegroundSecondaryLabel], [panelForegroundSecondaryLabel],
); );
return ( return (
<TouchableOpacity onPress={props.onPress}> <TouchableOpacity onPress={onCancel}>
<Text style={labelStyle}>Cancel</Text> <Text style={labelStyle}>Cancel</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
export default ChangeRolesHeaderLeftButton; export default ChangeRolesHeaderLeftButton;
View Options

services/backup/blob_client/src/lib.rs

// @flow // @flow
import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements'; import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Alert, Text } from 'react-native';
import { TouchableOpacity } from 'react-native-gesture-handler'; import { TouchableOpacity } from 'react-native-gesture-handler';
import type { NavigationRoute } from '../navigation/route-names';
import { useColors } from '../themes/colors.js'; import { useColors } from '../themes/colors.js';
type Props = React.ElementConfig<typeof BaseHeaderBackButton>; type ChangeRolesHeaderLeftButtonProps = {
function ChangeRolesHeaderLeftButton(props: Props): React.Node { +route: NavigationRoute<'ChangeRolesScreen'>,
...React.ElementConfig<typeof BaseHeaderBackButton>,
};
function ChangeRolesHeaderLeftButton(
props: ChangeRolesHeaderLeftButtonProps,
): React.Node {
const { memberInfo, role: selectedRole } = props.route.params;
const { role: memberRole } = memberInfo;
const onCancel = React.useCallback(() => {
const { onPress } = props;
invariant(onPress, 'onPress must be defined');
if (selectedRole === memberRole) {
onPress();
return;
}
Alert.alert(
'Unsaved Changes',
'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
[
{ text: 'Cancel', onPress },
{ text: 'Stay', style: 'cancel' },
],
{ cancelable: true },
);
}, [memberRole, props, selectedRole]);
rohanAuthorUnsubmitted
Done
Inline Actions

I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes.

Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted)

rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
ashoatUnsubmitted
Not Done
Inline Actions

What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted

ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
rohanAuthorUnsubmitted
Done
Inline Actions

Ah my bad. It looks like in message editing: Continue editing closes the alert and continues editing while Discard edit closes the alert and exits editing

rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
ashoatUnsubmitted
Not Done
Inline Actions
  1. I agree we shouldn't use the term Cancel here... how about Leave?
  2. I don't think we need { cancelable: true }... we can just default to Stay if the user dismisses the Alert
  3. Can we try to match copy for title / description with the edit-message workflow?
ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah that makes sense, I'll update the diff

rohan: Yeah that makes sense, I'll update the diff
const { panelForegroundSecondaryLabel } = useColors(); const { panelForegroundSecondaryLabel } = useColors();
const labelStyle = React.useMemo( const labelStyle = React.useMemo(
() => ({ () => ({
color: panelForegroundSecondaryLabel, color: panelForegroundSecondaryLabel,
marginLeft: 10, marginLeft: 10,
}), }),
[panelForegroundSecondaryLabel], [panelForegroundSecondaryLabel],
); );
return ( return (
<TouchableOpacity onPress={props.onPress}> <TouchableOpacity onPress={onCancel}>
<Text style={labelStyle}>Cancel</Text> <Text style={labelStyle}>Cancel</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
export default ChangeRolesHeaderLeftButton; export default ChangeRolesHeaderLeftButton;
View Options

services/backup/blob_client/src/put_client.rs

// @flow // @flow
import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements'; import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Alert, Text } from 'react-native';
import { TouchableOpacity } from 'react-native-gesture-handler'; import { TouchableOpacity } from 'react-native-gesture-handler';
import type { NavigationRoute } from '../navigation/route-names';
import { useColors } from '../themes/colors.js'; import { useColors } from '../themes/colors.js';
type Props = React.ElementConfig<typeof BaseHeaderBackButton>; type ChangeRolesHeaderLeftButtonProps = {
function ChangeRolesHeaderLeftButton(props: Props): React.Node { +route: NavigationRoute<'ChangeRolesScreen'>,
...React.ElementConfig<typeof BaseHeaderBackButton>,
};
function ChangeRolesHeaderLeftButton(
props: ChangeRolesHeaderLeftButtonProps,
): React.Node {
const { memberInfo, role: selectedRole } = props.route.params;
const { role: memberRole } = memberInfo;
const onCancel = React.useCallback(() => {
const { onPress } = props;
invariant(onPress, 'onPress must be defined');
if (selectedRole === memberRole) {
onPress();
return;
}
Alert.alert(
'Unsaved Changes',
'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
[
{ text: 'Cancel', onPress },
{ text: 'Stay', style: 'cancel' },
],
{ cancelable: true },
);
}, [memberRole, props, selectedRole]);
rohanAuthorUnsubmitted
Done
Inline Actions

I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes.

Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted)

rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
ashoatUnsubmitted
Not Done
Inline Actions

What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted

ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
rohanAuthorUnsubmitted
Done
Inline Actions

Ah my bad. It looks like in message editing: Continue editing closes the alert and continues editing while Discard edit closes the alert and exits editing

rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
ashoatUnsubmitted
Not Done
Inline Actions
  1. I agree we shouldn't use the term Cancel here... how about Leave?
  2. I don't think we need { cancelable: true }... we can just default to Stay if the user dismisses the Alert
  3. Can we try to match copy for title / description with the edit-message workflow?
ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah that makes sense, I'll update the diff

rohan: Yeah that makes sense, I'll update the diff
const { panelForegroundSecondaryLabel } = useColors(); const { panelForegroundSecondaryLabel } = useColors();
const labelStyle = React.useMemo( const labelStyle = React.useMemo(
() => ({ () => ({
color: panelForegroundSecondaryLabel, color: panelForegroundSecondaryLabel,
marginLeft: 10, marginLeft: 10,
}), }),
[panelForegroundSecondaryLabel], [panelForegroundSecondaryLabel],
); );
return ( return (
<TouchableOpacity onPress={props.onPress}> <TouchableOpacity onPress={onCancel}>
<Text style={labelStyle}>Cancel</Text> <Text style={labelStyle}>Cancel</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
export default ChangeRolesHeaderLeftButton; export default ChangeRolesHeaderLeftButton;
View Options

services/backup/blob_client/src/tools.rs

// @flow // @flow
import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements'; import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Alert, Text } from 'react-native';
import { TouchableOpacity } from 'react-native-gesture-handler'; import { TouchableOpacity } from 'react-native-gesture-handler';
import type { NavigationRoute } from '../navigation/route-names';
import { useColors } from '../themes/colors.js'; import { useColors } from '../themes/colors.js';
type Props = React.ElementConfig<typeof BaseHeaderBackButton>; type ChangeRolesHeaderLeftButtonProps = {
function ChangeRolesHeaderLeftButton(props: Props): React.Node { +route: NavigationRoute<'ChangeRolesScreen'>,
...React.ElementConfig<typeof BaseHeaderBackButton>,
};
function ChangeRolesHeaderLeftButton(
props: ChangeRolesHeaderLeftButtonProps,
): React.Node {
const { memberInfo, role: selectedRole } = props.route.params;
const { role: memberRole } = memberInfo;
const onCancel = React.useCallback(() => {
const { onPress } = props;
invariant(onPress, 'onPress must be defined');
if (selectedRole === memberRole) {
onPress();
return;
}
Alert.alert(
'Unsaved Changes',
'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
[
{ text: 'Cancel', onPress },
{ text: 'Stay', style: 'cancel' },
],
{ cancelable: true },
);
}, [memberRole, props, selectedRole]);
rohanAuthorUnsubmitted
Done
Inline Actions

I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes.

Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted)

rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
ashoatUnsubmitted
Not Done
Inline Actions

What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted

ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
rohanAuthorUnsubmitted
Done
Inline Actions

Ah my bad. It looks like in message editing: Continue editing closes the alert and continues editing while Discard edit closes the alert and exits editing

rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
ashoatUnsubmitted
Not Done
Inline Actions
  1. I agree we shouldn't use the term Cancel here... how about Leave?
  2. I don't think we need { cancelable: true }... we can just default to Stay if the user dismisses the Alert
  3. Can we try to match copy for title / description with the edit-message workflow?
ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah that makes sense, I'll update the diff

rohan: Yeah that makes sense, I'll update the diff
const { panelForegroundSecondaryLabel } = useColors(); const { panelForegroundSecondaryLabel } = useColors();
const labelStyle = React.useMemo( const labelStyle = React.useMemo(
() => ({ () => ({
color: panelForegroundSecondaryLabel, color: panelForegroundSecondaryLabel,
marginLeft: 10, marginLeft: 10,
}), }),
[panelForegroundSecondaryLabel], [panelForegroundSecondaryLabel],
); );
return ( return (
<TouchableOpacity onPress={props.onPress}> <TouchableOpacity onPress={onCancel}>
<Text style={labelStyle}>Cancel</Text> <Text style={labelStyle}>Cancel</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
export default ChangeRolesHeaderLeftButton; export default ChangeRolesHeaderLeftButton;