amaechler commented on code in PR #20619:
URL: https://github.com/apache/druid/pull/20619#discussion_r4301007908
##########
web-console/src/views/supervisors-view/supervisors-view.tsx:
##########
@@ -394,7 +395,7 @@ export class SupervisorsView extends React.PureComponent<
}
if (capabilities.hasOverlordAccess()) {
- if (visibleColumns.shown('Running tasks', 'Details', 'Recent
errors')) {
+ if (visibleColumns.shown('Running tasks', 'Partitions', 'Details',
'Recent errors')) {
Review Comment:
Since `Partitions` is visible by default, users who hid Running tasks,
Details and Recent errors to avoid the per-supervisor status calls get them
back after upgrading: their stored visibility has no `Partitions` key, so
`shown('Partitions')` is true. I reproduced this locally by setting
`supervisor-table-column-selection` to `{"Running tasks": false, "Details":
false, "Recent errors": false}`: the table shows Partitions and fires one
`/status` request per supervisor on every refresh, where master makes none.
Options: pass `['Partitions']` as `defaultHidden` to
`LocalStorageBackedVisibility`, or show the count inside the Details cell (see
the comment below), which avoids the new gate entry altogether.
##########
web-console/src/views/supervisors-view/supervisors-view.tsx:
##########
@@ -1049,6 +1050,26 @@ export class SupervisorsView extends React.PureComponent<
},
show: visibleColumns.shown('Running tasks'),
},
+ {
Review Comment:
But maybe easier than the above, have you considered putting this into
`Details` instead of a new column? Details already branches on
`kafka`/`kinesis`/`rabbit` and reads the same payload, so a second line
`Partitions: N` under `Lag: N` would be a few lines, keeps the table at 10
columns, avoids an always-empty column for autocompact and scheduled_batch
rows, and sidesteps the visibility issue above.
##########
web-console/src/views/supervisors-view/supervisors-view.tsx:
##########
@@ -1049,6 +1050,26 @@ export class SupervisorsView extends React.PureComponent<
},
show: visibleColumns.shown('Running tasks'),
},
+ {
+ Header: 'Partitions',
+ id: 'partitions',
+ accessor: 'supervisor_id',
+ width: 110,
+ filterable: false,
+ sortable: false,
+ className: 'padded',
+ show: visibleColumns.shown('Partitions'),
+ Cell: ({ value, original }) => {
+ const status = useContext(StatusContext);
+ if (!oneOf(original.type, 'kafka', 'kinesis', 'rabbit')) return
null;
+ const partitions = status[value]?.payload?.partitions;
+ return isNumberLike(partitions) ? (
+ <span data-tooltip="Number of partitions in the stream">
Review Comment:
nit: for Kinesis this is technically the shard count. If we'd know the
streaming type here, we could switch the terminology.
##########
web-console/src/views/supervisors-view/supervisors-view.tsx:
##########
@@ -1049,6 +1050,26 @@ export class SupervisorsView extends React.PureComponent<
},
show: visibleColumns.shown('Running tasks'),
},
+ {
+ Header: 'Partitions',
+ id: 'partitions',
+ accessor: 'supervisor_id',
+ width: 110,
+ filterable: false,
+ sortable: false,
+ className: 'padded',
+ show: visibleColumns.shown('Partitions'),
+ Cell: ({ value, original }) => {
+ const status = useContext(StatusContext);
+ if (!oneOf(original.type, 'kafka', 'kinesis', 'rabbit')) return
null;
Review Comment:
nit: this type check looks redundant. Only the seekable-stream report
payloads have a numeric `partitions`, so `isNumberLike(partitions)` already
returns `null` for other supervisor types. Dropping it also avoids a third copy
of the stream type list in this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
