-
Notifications
You must be signed in to change notification settings - Fork 17
feat: specify resource pools when launching a query #3198
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 files reviewed, 1 comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for specifying resource pools when launching queries in the YDB Embedded UI. Users can now select a resource pool from a dropdown in the query settings dialog, which will be applied to their query execution.
Key changes:
- Added resource pool selection to query settings dialog with support for fetching available pools
- Modified query execution to include
resource_poolparameter in API requests - Added utility function for applying resource pool pragma to queries (though not currently used)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/query.ts | Added RESOURCE_POOL_NO_OVERRIDE_VALUE constant and resourcePool field to query settings schema |
| src/types/api/query.ts | Added resource_pool parameter to SendQueryParams interface |
| src/store/reducers/query/utils.ts | Added applyResourcePoolPragma utility function to inject resource pool pragma into queries |
| src/store/reducers/query/query.ts | Added getResourcePools endpoint to fetch available pools and integrated resource_pool parameter into query execution |
| src/store/reducers/query/test/prepareQueryWithPragmas.test.ts | Added comprehensive tests for applyResourcePoolPragma function |
| src/containers/Tenant/Query/QuerySettingsDialog/i18n/ru.json | Added Russian translations for resource pool UI |
| src/containers/Tenant/Query/QuerySettingsDialog/i18n/en.json | Added English translations for resource pool UI |
| src/containers/Tenant/Query/QuerySettingsDialog/constants.ts | Added field settings for resource pool selector |
| src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsSelect.tsx | Updated type to support string values for resource pool names |
| src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsDialog.tsx | Added resource pool selector UI component with API integration |
src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsDialog.tsx
Outdated
Show resolved
Hide resolved
src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsDialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
src/utils/query.ts, line 309-317 (link)logic:
DEFAULT_QUERY_SETTINGSmissingresourcePoolfield. Should includeresourcePool: RESOURCE_POOL_NO_OVERRIDE_VALUEto ensure consistent initialization across the app.
6 files reviewed, 1 comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/utils/query.ts:317
- The
resourcePoolfield is missing fromDEFAULT_QUERY_SETTINGS. Since this is a new optional field that can be undefined, it should have a default value specified inDEFAULT_QUERY_SETTINGS(e.g.,resourcePool: undefinedorresourcePool: RESOURCE_POOL_NO_OVERRIDE_VALUE). This ensures consistent initialization and makes the default behavior explicit.
export const DEFAULT_QUERY_SETTINGS = {
queryMode: QUERY_MODES.query,
transactionMode: TRANSACTION_MODES.implicit,
timeout: null,
limitRows: 10000,
statisticsMode: STATISTICS_MODES.none,
tracingLevel: TRACING_LEVELS.off,
pragmas: defaultPragma,
};
src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsDialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/utils/query.ts:317
- The
DEFAULT_QUERY_SETTINGSobject should include a default value forresourcePoolto ensure consistent initialization. Consider addingresourcePool: RESOURCE_POOL_NO_OVERRIDE_VALUEto the defaults. This would prevent undefined values when settings are restored or initialized for the first time, ensuring the "No pool override" option is selected by default.
export const DEFAULT_QUERY_SETTINGS = {
queryMode: QUERY_MODES.query,
transactionMode: TRANSACTION_MODES.implicit,
timeout: null,
limitRows: 10000,
statisticsMode: STATISTICS_MODES.none,
tracingLevel: TRACING_LEVELS.off,
pragmas: defaultPragma,
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11 files reviewed, no comments
astandrik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should get resource pools on query editor opening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11 files reviewed, 1 comment
src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsDialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 files reviewed, 2 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 files reviewed, 1 comment
|
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!
| version: 8, | ||
| result: [ | ||
| { | ||
| rows: [['default'], ['olap']], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not await setupResourcePoolMock(page, ['default', 'olap'])?
| }); | ||
| }); | ||
|
|
||
| await page.route(`${backend}/viewer/query?*`, async (route: Route) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have this code twice?
await page.route(`${backend}/viewer/json/query?*`, async (route: Route) => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because streamQuery uses /viewer/query while sendQuery uses /viewer/json/query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 files reviewed, 2 comments
...ainers/Tenant/Query/QueryEditorControls/utils/getChangedQueryExecutionSettingsDescription.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
13 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 files reviewed, no comments
Closes #3134
Stand
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ✨3 ⏭️2
✨ New Tests (3)
⏭️ Skipped Tests (2)
Bundle Size: 🔺
Current: 62.50 MB | Main: 62.48 MB
Diff: +0.01 MB (0.02%)
ℹ️ CI Information