Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(compass-indexes): poll rolling indexes COMPASS-8217 #6290

Closed
wants to merge 9 commits into from
Closed

feat(compass-indexes): poll rolling indexes COMPASS-8217 #6290

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ef4627c
WIP
lerouxb Sep 26, 2024
503bf7c
Merge branch 'main' into poll-rolling-indexes
lerouxb Sep 26, 2024
ac08457
display all three lists of indexes
lerouxb Sep 26, 2024
35608a8
fixes
lerouxb Sep 26, 2024
815d8d9
remove extra
lerouxb Sep 26, 2024
665eada
tweaks
lerouxb Sep 26, 2024
9d0fdd7
fixes
lerouxb Sep 27, 2024
3eba245
tests
lerouxb Sep 27, 2024
dd13789
export the feature functions separately for use outside of hooks
lerouxb Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions packages/atlas-service/src/make-automation-agent-op-request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
export type AtlasIndexStats = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to comment on this, i randomly clicked on the pr and it caught my eye, definitely a nit, and a question also for @gribnoysup.

Isn't this inconsistent with the rest of the design of AtlasService? For the way it is setup for other calls we shouldn't have domain related concepts and types on the atlas service directly, and they should live inside the services that are related to those domains.

It should all still work with the generic, just the definition of the type doesn't seem to fit the rest of the design. Unless something changed recently.

Copy link
Collaborator

@gribnoysup gribnoysup Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Le Roux is just moving types here, so it's coming from my initial implementation of the automation agent request helper, so you can definitely blame me for that 😄

I think it's fair that this is maybe giving this service too much knowledge about the API, but the API is not very consistent and requires to know more about what commands specifically are being executed if we want to have a single way of doing these two-part agent requests. See here for example, certain types of automation agent requests don't return a request id we can wait on, so we need a narrow typing to know what are those cases, the request params and the way response is wrapped in await is also based on the command you're sending, etc.

We can change this to a generic type and move narrower domain specific stuff to rolling indexes, we'd also need to make the assertions we're doing inside the method configurable from outside, or just dismantle this helper in two methods that just know the URL shape for creating these requests and then we re-assemble it into the same logic separately for global writes and rolling indexes. Not very "dry", but it's also just two features we need this for right now, so might be worth it to remove the types from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually have a bunch of scattered TODO comments/reminders to talk about this. This type doesn't map well to our own index type at all and I'm not 100% sure where we should do the mapping/assertions.

Copy link
Collaborator

@mcasimir mcasimir Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies if i'm misunderstanding the code, looking at

compass/packages/atlas-service/src/make-automation-agent-op-request.ts

Lines 168 to 173 in d82bfb5

// Rolling index creation request doesn't return anything that we can "await"
// on (a successful response is already an acknowledgement that request to
// create an index was registered), so we just end here
if (opType === 'index') {
return undefined as AutomationAgentResponse<OpType>;
}
making this behaviour configurable from outside and externalizing the type seems definitely the right thing to do (assuming that we want to keep AtlasService as a building block for domain specific services).

The fact that you need ifs by "opType" there means that we are mixing up responsibilities already, and is not obvious at all what is happening there. It looks also pretty simple to just split that method in two makeAutomationAgentOpRequest<RT> and makeAutomationAgentOpRequestAndWait<RT> so we don't rely on too many parameters and ifs.

can definitely blame me for that

No blame at all!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we'll split it into parts

cc @mabaasit just FYI you'd need to account for that for Global Writes implementation

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #6305 to make the split

collName: string;
dbName: string;
indexName: string;
indexProperties: { label: string; properties: Record<string, unknown> }[];
indexType: { label: string };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that it is { label } and not immediately just the string. Trying to figure out where would be the ideal place to turn that string into an exact index type.

keys: { name: string; value: string | number };
sizeBytes: number;
status: 'rolling build' | 'building' | 'exists';
};

type ClusterOrServerlessId =
| { serverlessId?: never; clusterId: string }
| { serverlessId: string; clusterId?: never };
Expand Down Expand Up @@ -49,16 +60,7 @@ function assertAutomationAgentRequestResponse<
}

export type AutomationAgentAwaitResponseTypes = {
listIndexStats: {
collName: string;
dbName: string;
indexName: string;
indexProperties: { label: string; properties: Record<string, unknown> }[];
indexType: { label: string };
keys: { name: string; value: string | number };
sizeBytes: number;
status: 'rolling build' | 'building' | 'exists';
}[];
listIndexStats: AtlasIndexStats[];
dropIndex: never[];
};

Expand Down
1 change: 1 addition & 0 deletions packages/atlas-service/src/provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,4 @@ export const atlasServiceLocator = createServiceLocator(
export { AtlasAuthService } from './atlas-auth-service';
export type { AtlasService } from './atlas-service';
export type { AtlasUserInfo } from './renderer';
export type { AtlasIndexStats } from './make-automation-agent-op-request.ts';
16 changes: 13 additions & 3 deletions packages/compass-indexes/src/components/regular-indexes-table/index-actions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,17 @@ import semver from 'semver';
import React, { useCallback, useMemo } from 'react';
import type { GroupedItemAction } from '@mongodb-js/compass-components';
import { ItemActionGroup } from '@mongodb-js/compass-components';
import type { RegularIndex } from '../../modules/regular-indexes';

type IndexActionsIndex = {
name: string;
compassIndexType: 'regular-index' | 'in-progress-index' | 'rolling-index';
extra?: {
hidden?: boolean;
};
};

type IndexActionsProps = {
index: RegularIndex;
index: IndexActionsIndex;
serverVersion: string;
onDeleteIndexClick: (name: string) => void;
onHideIndexClick: (name: string) => void;
Expand Down Expand Up @@ -40,7 +47,10 @@ const IndexActions: React.FunctionComponent<IndexActionsProps> = ({
},
];

if (serverSupportsHideIndex(serverVersion)) {
if (
index.compassIndexType === 'regular-index' &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug - we showed the hide button for in-progress indexes.

serverSupportsHideIndex(serverVersion)
) {
actions.unshift(
index.extra?.hidden
? {
Expand Down
Loading
Loading