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

lerouxb · 2024-09-26T15:30:19Z

Very much in-progress.

Thinking is to have real/regular indexes, in-progress regular indexes and rolling indexes as three separate lists with no merging happening in the store. Then to do any merging / concatenation that's necessary in the table only.

The problem is that the three things' types are very different. So even then it still leaves the question of whether to map to some common type in the store side or only in the UI. And what to do with all the columns' components: At which point does the component take all three types vs just creating specialised components rather?

lerouxb · 2024-09-26T15:33:00Z

packages/compass-indexes/src/components/regular-indexes-table/index-actions.tsx

@@ -40,7 +47,10 @@ const IndexActions: React.FunctionComponent<IndexActionsProps> = ({
      },
    ];

-    if (serverSupportsHideIndex(serverVersion)) {
+    if (
+      index.compassIndexType === 'regular-index' &&


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

lerouxb · 2024-09-26T16:02:43Z

packages/compass-indexes/src/components/regular-indexes-table/index-actions.tsx

@@ -58,6 +64,19 @@ const IndexActions: React.FunctionComponent<IndexActionsProps> = ({
      );
    }

+    // you can only drop regular indexes or failed inprogress indexes


This was also a bug - you could drop an in progress index that was really still in progress and that just wouldn't work because the index creation isn't failed and it isn't a real index yet either.

lerouxb · 2024-09-26T16:08:12Z

packages/compass-indexes/src/modules/regular-indexes.ts


+    // TODO: this should be its own function, not part of dropIndex


There was actually momentarily a bug here. Because I'm not merging the in-progress indexes into indexes, the code:

const index = indexes.find((x) => x.name === indexName); if (!index) { return; }

which ran first meant that you could never get to the check to see if the thing is an in-progress index. Which wasn't as obvious to find and I think if the two things weren't both rolled into dropIndex() then it would have been easier to spot, more likely to be tested, etc.

lerouxb · 2024-09-26T16:10:49Z

packages/compass-indexes/src/components/regular-indexes-table/regular-indexes-table.tsx

  type: React.ReactNode;
  size: React.ReactNode;
-  usage: React.ReactNode;
+  usageCount: React.ReactNode;


We were changing usageCount into usage back into usageCount..

lerouxb · 2024-09-26T16:20:44Z

packages/compass-indexes/src/components/regular-indexes-table/regular-indexes-table.tsx

+    // TODO: check that label really is a regular index type
+    type: (
+      <TypeField
+        type={index.indexType.label as RegularIndex['type']}


Would be nice if the type could already be narrowed from string to a regular index type somehow.

lerouxb · 2024-09-26T16:22:05Z

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

+  dbName: string;
+  indexName: string;
+  indexProperties: { label: string; properties: Record<string, unknown> }[];
+  indexType: { label: string };


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.

mcasimir · 2024-09-27T15:04:35Z

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

@@ -1,3 +1,14 @@
+export type AtlasIndexStats = {


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.

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.

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.

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!

Okay, we'll split it into parts

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

Opened #6305 to make the split

lerouxb · 2024-09-30T12:58:54Z

Closing in favour of #6300

lerouxb added 4 commits September 26, 2024 08:58

WIP

ef4627c

Merge branch 'main' into poll-rolling-indexes

503bf7c

display all three lists of indexes

ac08457

fixes

35608a8

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

lerouxb added the release notes label Sep 26, 2024

lerouxb commented Sep 26, 2024

View reviewed changes

remove extra

815d8d9

lerouxb commented Sep 26, 2024

View reviewed changes

lerouxb added 3 commits September 26, 2024 17:24

tweaks

665eada

fixes

9d0fdd7

tests

3eba245

mcasimir reviewed Sep 27, 2024

View reviewed changes

export the feature functions separately for use outside of hooks

dd13789

lerouxb mentioned this pull request Sep 30, 2024

chore(compas-indexes): store real indexes and in-progress indexes as separate lists #6300

Merged

lerouxb closed this Sep 30, 2024

lerouxb deleted the poll-rolling-indexes branch September 30, 2024 12:58

gribnoysup mentioned this pull request Sep 30, 2024

chore(atlas-service, indexes): remove all the domain specific parts of automation agent request helper #6305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lerouxb commented Sep 26, 2024 •

edited

Loading

lerouxb Sep 26, 2024

lerouxb Sep 26, 2024

lerouxb Sep 26, 2024 •

edited

Loading

lerouxb Sep 26, 2024 •

edited

Loading

lerouxb Sep 26, 2024

lerouxb Sep 26, 2024

mcasimir Sep 27, 2024

gribnoysup Sep 27, 2024 •

edited

Loading

lerouxb Sep 27, 2024

mcasimir Sep 30, 2024 •

edited

Loading

gribnoysup Sep 30, 2024

gribnoysup Sep 30, 2024

lerouxb commented Sep 30, 2024


		// TODO: this should be its own function, not part of dropIndex

	// 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>;
	}

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

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

Conversation

lerouxb commented Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lerouxb Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

lerouxb 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

gribnoysup Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcasimir Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lerouxb commented Sep 30, 2024

lerouxb commented Sep 26, 2024 •

edited

Loading

lerouxb Sep 26, 2024 •

edited

Loading

lerouxb Sep 26, 2024 •

edited

Loading

gribnoysup Sep 27, 2024 •

edited

Loading

mcasimir Sep 30, 2024 •

edited

Loading