feat(global-writes): add unsharded state COMPASS-8276 #6289

mabaasit · 2024-09-26T11:57:07Z

This PR includes:

Warning icon on Global Writes tab (along with a tooltip);
Unsharded state which allows users to create a shard key for a collection;
Initial sharding state, which is shown once server starts to create a shard key

Preview

Tab Warning

Unsharded State

Sharding State (more work in next ticket)

Description

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

…PASS-8275-setup-global-writes-plugin

…PASS-8276-no-shard-key

mabaasit · 2024-09-26T12:00:20Z

packages/compass-global-writes/src/store/reducer.ts

+  createShardkey: {
+    isLoading: boolean;
+  };


This was not part of the tech doc, but while working on this, I realzied we need to disable the create-shard-button and added this. as an alternative, we can also await on the dispatch action on the react side and while its waiting we disable the button.

Can you clarify why is this not just part of sharding status? I'm maybe missing the purpose, but this seems like just part of the status transition for creation flow, so why are we moving this state into its own field?

this is a state between UNSHARDED and SHARDING, time duration for which user clicks the create shard key and server acknowledges the change - keeps the button disabled, and for error shows the toast.

So this loading state is mutually exclusive with all the other states, is that correct? If yes, in that case we should just add it as another state in list of available state transitions for the component, using multiple states to declare mutually exclusive, state-machine-like, states of the application can lead to potentially weird cases and hard to reason about logic as it basically allows application to be in multiple states at once.

Yes that's correct. It does make sense to add these to the state as well, its just that I think the list will grow a little and in such case, i find it a bit hard to maintain the transitions.

But I will try to do that. Thanks for the suggestion.

added in 23c1361

mabaasit · 2024-09-26T12:01:43Z

packages/compass-global-writes/src/store/reducer.ts

+        });
+        return;
+      }
+      // TODO (COMPASS-8277): Now fetch the sharding key and possible process error.


Here in the follow up tickets, we want to fetch the shard keys and also the agent process errors and based on that information, transition to other states. Currently this leaves user in a loading state, but i think that's fine with the current state of the project.

mabaasit · 2024-09-26T12:03:17Z

packages/compass-global-writes/src/store/reducer.ts

+      openToast('global-writes-fetch-shard-info-error', {
+        title: `Failed to fetch sharding information: ${
+          (error as Error).message
+        }`,
+        dismissible: true,
+        timeout: 5000,
+        variant: 'important',
+      });
+    }


we also want to have another state like ERROR or something, where user does not see loading and can see the error and possibly an action to refetch.

mabaasit · 2024-09-26T12:04:26Z

packages/compass-global-writes/src/services/atlas-global-writes-service.ts

 import type { AtlasService } from '@mongodb-js/atlas-service/provider';
+import type { CreateShardKeyData } from '../store/reducer';
+
+type ZoneMapping = unknown;


leaving it unknow for now, will implement it when needed

addaleax · 2024-09-26T12:07:24Z

packages/compass-global-writes/src/components/index.tsx

+    case ShardingStatuses.NOT_READY:
+      return (
+        <div className={centeredContent}>
+          <SpinLoaderWithLabel progressText="Loading ..." />


Suggested change

<SpinLoaderWithLabel progressText="Loading ..." />

<SpinLoaderWithLabel progressText="Loading …" />

addaleax · 2024-09-26T12:07:51Z

packages/compass-global-writes/src/components/states/sharding.tsx

+  return (
+    <div className={containerStyles}>
+      <Banner variant={BannerVariant.Info}>
+        <strong>Sharding your collection ...</strong>


Suggested change

<strong>Sharding your collection ...</strong>

<strong>Sharding your collection …</strong>

(or I guess you can … like the ' below – but just fyi, ' is just a literal ' that you don't really need to escape here anyway. And even more pedantically, you'd want to use ‘ ’ instead of ' 😄)

added in 3b384fc

addaleax · 2024-09-26T12:11:47Z

packages/compass-global-writes/src/store/reducer.ts

+        'AtlasFetchError',
+        'Error creating cluster shard key',
+        {
+          error: error as Error,


I don't think we can log literal Error objects?

removed to log the message only

gribnoysup · 2024-09-26T12:52:37Z

packages/compass-global-writes/src/store/reducer.ts

+>;
+
+enum GlobalWritesActionTypes {
+  SetIsManagedNamespace = 'global-writes/SetIsManagedNamespace',


Avoid setter actions. What is the actual event that happens in the app here?

fixed in a0eb2be.

this action is dispatched after we call the api to check if the namespace is managed or not, in fetchClusterShardingData action.

gribnoysup · 2024-09-26T12:58:32Z

packages/compass-global-writes/src/components/index.tsx

+function getStateViewBasedOnShardingStatus(shardingStatus: ShardingStatus) {
+  switch (shardingStatus) {
+    case ShardingStatuses.NOT_READY:
+      return (
+        <div className={centeredContent}>
+          <SpinLoaderWithLabel progressText="Loading …" />
+        </div>
+      );
+    case ShardingStatuses.UNSHARDED:
+      return <UnshardedState />;
+    case ShardingStatuses.SHARDING:
+      return <ShardingState />;
+    default:
+      return null;
+  }
+}


Nit: this is just a React component that is written like a plain function, I'd suggest we don't do that because you loose some React functionality that might come in handy like memoing the component or the ability to safely use hooks inside this function

fixed in 5f03a49

gribnoysup · 2024-09-26T13:01:39Z

packages/compass-global-writes/src/store/reducer.ts

+  createShardkey: {
+    isLoading: boolean;
+  };


Can you clarify why is this not just part of sharding status? I'm maybe missing the purpose, but this seems like just part of the status transition for creation flow, so why are we moving this state into its own field?

packages/compass-global-writes/src/plugin-title.tsx

packages/compass-global-writes/src/index.ts

packages/compass-global-writes/src/components/index.spec.tsx

paula-stacho · 2024-09-27T11:52:58Z

Perhaps we could add some left padding, so that the LG hover visual isn't cut off:

I'm thinking either 16px on the checkboxes (so that they are aligned with the text, skipping the toggle icon), or 4px on the form and description. What do you think?

packages/compass-global-writes/src/store/reducer.ts

mabaasit · 2024-09-27T12:37:26Z

Perhaps we could add some left padding, so that the LG hover visual isn't cut off: I'm thinking either 16px on the checkboxes (so that they are aligned with the text, skipping the toggle icon), or 4px on the form and description. What do you think?

I added some spacing to avoid this in eca4068 (but only for the radio options and aligned checkbox accordingly)

paula-stacho

Looks good!

mabaasit added 24 commits September 23, 2024 13:26

setup plugin

1342fae

service and clean up

5c92d42

Merge branch 'main' of https://github.com/mongodb-js/compass into COM…

8680d5d

…PASS-8275-setup-global-writes-plugin

show tab only if global writes is supported

7b1a784

clean up

17fa0dd

fix ts

16c5289

Merge branch 'main' of https://github.com/mongodb-js/compass into COM…

1dfed32

…PASS-8275-setup-global-writes-plugin

clean up

f64ea1d

depcheck

3b74cf6

correct version of redux in lock file

2c4f6ff

depcheck again

15fb5c0

feedback

22ff79f

warning

b1f5d56

Merge branch 'main' of https://github.com/mongodb-js/compass into COM…

abf38b9

…PASS-8276-no-shard-key

show warning

ca0afc7

title tooltip

1cef989

unsharded view and tests

e6788d6

sharding in progress

e4ab4a4

small clean up

d6790e7

correct nums

6e2c893

error toasts

99a5529

fix html

84fef30

tests

f644bd7

Merge branch 'main' of https://github.com/mongodb-js/compass into COM…

4f0ce46

…PASS-8276-no-shard-key

github-actions bot added the feat label Sep 26, 2024

mabaasit commented Sep 26, 2024

View reviewed changes

mabaasit added the no release notes Fix or feature not for release notes label Sep 26, 2024

addaleax reviewed Sep 26, 2024

View reviewed changes

mabaasit added 2 commits September 26, 2024 14:15

clean up

9e3be02

pedantic details

3b384fc

mabaasit added 2 commits September 26, 2024 14:50

add missing header

1c8897a

check

e8c55b0

gribnoysup reviewed Sep 26, 2024

View reviewed changes

mabaasit added 3 commits September 26, 2024 15:08

clean up lock file

d7745cd

clean action name

a0eb2be

func to component

5f03a49

mabaasit marked this pull request as ready for review September 26, 2024 13:21

mabaasit added 5 commits September 26, 2024 16:10

fix check

9437e07

use state when waiting for server to accept sharding

23c1361

name fix

c8be176

clean up

0d26155

assert as correct type

efbd45f

paula-stacho reviewed Sep 27, 2024

View reviewed changes

packages/compass-global-writes/src/plugin-title.tsx Show resolved Hide resolved

paula-stacho reviewed Sep 27, 2024

View reviewed changes

packages/compass-global-writes/src/index.ts Show resolved Hide resolved

paula-stacho reviewed Sep 27, 2024

View reviewed changes

packages/compass-global-writes/src/components/index.spec.tsx Show resolved Hide resolved

mabaasit added 2 commits September 27, 2024 12:03

add test

59787bb

remove assertion

74ff479

paula-stacho reviewed Sep 27, 2024

View reviewed changes

packages/compass-global-writes/src/store/reducer.ts Show resolved Hide resolved

mabaasit added 2 commits September 27, 2024 14:23

add back assertion

07cce5f

avoid cutting off radio buttons

eca4068

Merge branch 'main' into COMPASS-8276-no-shard-key

10adbef

paula-stacho approved these changes Sep 27, 2024

View reviewed changes

mabaasit added 2 commits September 27, 2024 16:05

Merge remote-tracking branch 'origin' into COMPASS-8276-no-shard-key

915e8a3

npm i

373bffe

mabaasit merged commit b9957ff into main Sep 27, 2024
27 checks passed

mabaasit deleted the COMPASS-8276-no-shard-key branch September 27, 2024 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(global-writes): add unsharded state COMPASS-8276 #6289

feat(global-writes): add unsharded state COMPASS-8276 #6289

mabaasit commented Sep 26, 2024 •

edited

Loading

mabaasit Sep 26, 2024

gribnoysup Sep 26, 2024

mabaasit Sep 26, 2024

gribnoysup Sep 26, 2024

mabaasit Sep 26, 2024

mabaasit Sep 26, 2024

mabaasit Sep 26, 2024

mabaasit Sep 26, 2024 •

edited

Loading

mabaasit Sep 26, 2024

addaleax Sep 26, 2024

addaleax Sep 26, 2024

mabaasit Sep 26, 2024

addaleax Sep 26, 2024

mabaasit Sep 26, 2024

gribnoysup Sep 26, 2024

mabaasit Sep 26, 2024

gribnoysup Sep 26, 2024

mabaasit Sep 26, 2024

gribnoysup Sep 26, 2024

paula-stacho commented Sep 27, 2024 •

edited

Loading

mabaasit commented Sep 27, 2024 •

edited

Loading

paula-stacho left a comment

	<SpinLoaderWithLabel progressText="Loading ..." />
	<SpinLoaderWithLabel progressText="Loading …" />

	<strong>Sharding your collection ...</strong>
	<strong>Sharding your collection …</strong>

feat(global-writes): add unsharded state COMPASS-8276 #6289

feat(global-writes): add unsharded state COMPASS-8276 #6289

Conversation

mabaasit commented Sep 26, 2024 • edited Loading

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabaasit Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paula-stacho commented Sep 27, 2024 • edited Loading

mabaasit commented Sep 27, 2024 • edited Loading

paula-stacho left a comment

Choose a reason for hiding this comment

mabaasit commented Sep 26, 2024 •

edited

Loading

mabaasit Sep 26, 2024 •

edited

Loading

paula-stacho commented Sep 27, 2024 •

edited

Loading

mabaasit commented Sep 27, 2024 •

edited

Loading