-
Notifications
You must be signed in to change notification settings - Fork 18
feat: add partitions progress bar to table info #3206
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.
6 files reviewed, 1 comment
src/containers/Tenant/Diagnostics/Overview/TableInfo/prepareTableInfo.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.
7 files reviewed, no comments
src/containers/Tenant/Diagnostics/Overview/TableInfo/PartitionsProgress/helpers.ts
Show resolved
Hide resolved
|
|
||
| // Feature flag: show partitions progress only if WINDOW_SHOW_TABLE_SETTINGS is truthy | ||
| const isPartitionsProgressEnabled = Boolean( | ||
| (window as unknown as {WINDOW_SHOW_TABLE_SETTINGS?: unknown}).WINDOW_SHOW_TABLE_SETTINGS, |
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.
I think we could add type to window.d.ts to avoid "as unknown"
| title={title} | ||
| className={b('info-block')} | ||
| renderEmptyState={() => <div className={b('title')}>{title}</div>} | ||
| renderEmptyState={() => null} |
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.
Is returning null instead of message intentional?
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.
Yes, it is intentional)
Previously we used renderEmptyState to render the block title when there was no data, because the title was inside InfoViewer. After implementing a progress bar TableInfo renders a permanent header above the whole block. If I return a title from renderEmptyState now, we will have two titles for the same section in "no data" case, which looks redundant.
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.
Left couple of questions
| "tabletMetrics": "Tablet Metrics", | ||
| "title": "Partitioning", | ||
| "tableStats": "Stats", | ||
| "tabletMetrics": "Metrics", |
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.
it would be great to create new keys in i18n-naming-ruleset.md format
(and very super best to change old keys to use it =)) )
|
|
||
| const hasMaxLimit = !isNil(maxPartitions); | ||
|
|
||
| if (!hasMaxLimit) { |
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.
Would be good to move calculations to some function (maybe utils.ts in the same folder) and to cover it with unit tests
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
The partitions progress bar is hidden behind a window flag for now.
To enable it in the browser, run in DevTools console:
window.WINDOW_SHOW_TABLE_SETTINGS = true;Stand: https://nda.ya.ru/t/tOLLKQmr7PaJQ5
In this database you can see different tables with different number of partitions (with min, max, less then min, with no max)
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔺
Current: 62.50 MB | Main: 62.48 MB
Diff: +0.02 MB (0.03%)
ℹ️ CI Information