feat(ingestion): add Executors tab with create/delete executor pools#364
feat(ingestion): add Executors tab with create/delete executor pools#364bmiller-dh wants to merge 3 commits intomasterfrom
Conversation
- Executors tab on Manage Data Sources with table, checkboxes, Create pool and Delete Pool(s) - Delete guarded when ingestion sources use selected pools (modal shows which sources, Close only) - GraphQL: listExecutorPools, createExecutorPool, deleteExecutorPools; Java resolvers with in-memory store - ConfirmationModal: hideConfirmButton for blocked state; theme fix for useSetAppTheme (import.meta.glob) - Docs: Managing Executor Pools section in docs/ui-ingestion.md - Unit tests for ExecutorPoolsList, ExecutorPoolsTab, CreateExecutorPoolModal
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| query: undefined, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Delete guard fails with more than 1000 sources
Medium Severity
The delete guard feature that prevents deleting pools in use by ingestion sources only checks the first 1000 sources (SOURCES_FETCH_COUNT = 1000). If a deployment has more than 1000 ingestion sources, pools used by sources beyond that limit won't be detected as "in use", allowing users to delete pools that are actually still being used. This breaks the safety guarantee described in the PR where "Pools in use by ingestion sources cannot be deleted."
...n/java/com/linkedin/datahub/graphql/resolvers/ingest/executor/ListExecutorPoolsResolver.java
Outdated
Show resolved
Hide resolved
| ExecutorPool pool = new ExecutorPool(); | ||
| pool.setId(id.trim()); | ||
| pool.setName(name != null && !name.isBlank() ? name.trim() : null); | ||
| POOLS.put(id.trim(), pool); |
There was a problem hiding this comment.
Creating pool with existing ID silently overwrites it
Medium Severity
The create method uses POOLS.put() which silently overwrites any existing pool with the same ID. When a user calls the createExecutorPool mutation with an ID that already exists, the existing pool's name is replaced without any error or warning. Users expect a "create" operation to fail if the resource already exists, not perform an upsert. This could lead to accidental data loss if a user unknowingly provides an ID that's already in use.
| int from = Math.min(start, total); | ||
| int to = Math.min(start + count, total); | ||
| return new ArrayList<>(all.subList(from, to)); | ||
| } |
There was a problem hiding this comment.
Negative pagination values cause IndexOutOfBoundsException crash
Medium Severity
The list() method passes user-provided start and count values directly to ArrayList.subList() without validating they're non-negative. A GraphQL query with start: -1 would result in subList(-1, ...) which throws IndexOutOfBoundsException. Similarly, a negative count could make to less than from, throwing IllegalArgumentException. This would cause the API to return a 500 error instead of handling invalid input gracefully.
Additional Locations (1)
Bundle ReportChanges will increase total bundle size by 5.79kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
| int total = all.size(); | ||
| int from = Math.min(start, total); | ||
| int to = Math.min(start + count, total); | ||
| return new ArrayList<>(all.subList(from, to)); |
There was a problem hiding this comment.
Unpredictable pool ordering breaks pagination consistency
Low Severity
The ExecutorPoolStore uses ConcurrentHashMap which does not preserve insertion order or provide any deterministic iteration order. Pools created as A, B, C may be returned in any order (e.g., C, A, B), and this order may change between requests. This makes server-side pagination unreliable—a pool on page 1 in one request might appear on page 2 in the next.
- Remove executor pool schema, resolvers, and store from graphql-core - Add E2E test for Executors tab (ingestionV2/executors.js) - Backend for list/create/delete executor pools lives on target branch
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| }); | ||
| setSelectedPoolIds([]); | ||
| setShowDeleteConfirm(false); | ||
| refetch?.(); |
There was a problem hiding this comment.
Page state not reset after deletion causes empty view
Medium Severity
After successfully deleting pools, selectedPoolIds is cleared and refetch() is called, but the page state is never reset. If a user is on page 2 and deletes all items on that page, after refetch the current page will be empty (pools.slice(25, 50) returns []). Because totalPools > 0, the empty state component won't render, and with hideOnSinglePage the pagination is hidden when only one page of data remains. The user sees an empty table with no visible way to navigate back to page 1.
Additional Locations (2)
| query: undefined, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Delete guard silently fails when sources query errors
Medium Severity
The useListIngestionSourcesQuery only destructures data, ignoring error and loading. If this query fails (network error, server error, etc.), sourcesData is undefined, causing sourcesByExecutorId to be an empty object. This makes the delete guard silently non-functional—poolsInUse will always be empty, deleteBlocked will always be false, and users can delete pools that are actually in use by ingestion sources without any warning. The PR explicitly describes this guard as a key feature.
1 participant
Summary
listExecutorPools,createExecutorPool,deleteExecutorPools; the backend that implements these lives on the target branch (e.g. acryl-main).ingestionV2/executors.js— navigates to Executors tab and verifies toolbar (Create pool, Delete Pool(s)).docs/ui-ingestion.mdupdated with "Managing Executor Pools (Executors tab)".Checklist
Note
Medium Risk
Adds new ingestion UI flows and GraphQL mutations for creating/deleting executor pools, plus a shared modal API change that could affect other confirmation dialogs if misused; theme-loader change is localized but impacts app startup in dev.
Overview
Adds a new Executors tab to Manage Data Sources that lists executor pools and supports creating pools (via a modal requiring Pool ID + optional name) and bulk deletion with selection and pagination.
Deletion is guarded by checking ingestion sources’
executorIdusage: the confirmation dialog becomes informational and blocks deletion when selected pools are in use (showing dependent sources).Also introduces GraphQL operations
listExecutorPools,createExecutorPool, anddeleteExecutorPools, updates ingestion tab routing (/ingestion/executors), adds docs + Cypress coverage, and extendsConfirmationModalwithhideConfirmButton; separately hardens dev-time custom theme loading by switching toimport.meta.globfor JSON themes.Written by Cursor Bugbot for commit ad4f0e0. This will update automatically on new commits. Configure here.