ccaominh commented on a change in pull request #10438:
URL: https://github.com/apache/druid/pull/10438#discussion_r496187642
##########
File path: web-console/src/components/more-button/more-button.tsx
##########
@@ -18,14 +18,19 @@
import { Button, Menu, Popover, Position } from '@blueprintjs/core';
import { IconNames } from '@blueprintjs/icons';
-import React from 'react';
+import React, { useState } from 'react';
+
+type OpenState = 'open' | 'alt-open';
export interface MoreButtonProps {
- children: React.ReactNode;
+ children: React.ReactNode | React.ReactNode[];
+ altExtra?: React.ReactNode;
Review comment:
What do you think about adding another snapshot test that sets the new
`altExtra` prop?
##########
File path: web-console/src/utils/general.tsx
##########
@@ -231,6 +231,10 @@ export function formatMegabytes(n: number): string {
return numeral(n / 1048576).format('0,0.0');
}
+export function formatPercent(n: number): string {
+ return (n * 100).toFixed(2) + '%';
+}
Review comment:
Missing unit test in `general.spec.ts`
##########
File path: web-console/src/views/datasource-view/datasource-view.tsx
##########
@@ -329,18 +393,25 @@ GROUP BY 1`;
const rulesResp = await axios.get('/druid/coordinator/v1/rules');
const rules = rulesResp.data;
- const compactionResp = await
axios.get('/druid/coordinator/v1/config/compaction');
- const compaction = lookupBy(
- compactionResp.data.compactionConfigs,
- (c: any) => c.dataSource,
+ const compactionConfigsResp = await
axios.get('/druid/coordinator/v1/config/compaction');
+ const compactionConfigs = lookupBy(
+ compactionConfigsResp.data.compactionConfigs || [],
+ (c: CompactionConfig) => c.dataSource,
+ );
+
+ const compactionStatusesResp = await
axios.get('/druid/coordinator/v1/compaction/status');
+ const compactionStatuses = lookupBy(
+ compactionStatusesResp.data.latestStatus || [],
+ (c: CompactionStatus) => c.dataSource,
Review comment:
This will be hard to unit test since it requires either a real or mock
druid cluster. One option to make it more testable is to restructure the code
to inject something like a compaction manager dependency via the
`DatasourcesView` constructor. That way, the unit tests can have a compaction
manager mock where the API responses are stubbed.
https://github.com/apache/druid/pull/9956 used this approach to test
`SegmentTimeline` with a mock for `QueryManager`.
##########
File path: web-console/src/views/datasource-view/datasource-view.tsx
##########
@@ -552,7 +634,32 @@ GROUP BY 1`;
);
}
- private saveRules = async (datasource: string, rules: any[], comment:
string) => {
+ renderForceCompactAction() {
+ const { showForceCompact } = this.state;
+ if (!showForceCompact) return;
+
+ return (
+ <AsyncActionDialog
+ action={async () => {
+ const resp = await
axios.post(`/druid/coordinator/v1/compaction/compact`, {});
+ return resp.data;
+ }}
+ confirmButtonText="Force compaction run"
+ successText="Out of band compaction run has been initiated"
+ failText="Could not force compaction"
+ intent={Intent.DANGER}
+ onClose={() => {
+ this.setState({ showForceCompact: false });
+ }}
+ >
+ <p>Are you sure you want to force a compaction run?</p>
+ <p>This functionality only exists for debugging and testing
reasons.</p>
+ <p>If you are running it in production you are doing something
wrong.</p>
+ </AsyncActionDialog>
Review comment:
Currently this is hard to test. The autocompaction E2E test could
possibly be modified to use this instead of calling the trigger compaction API
directly.
##########
File path: web-console/src/views/datasource-view/datasource-view.tsx
##########
@@ -131,9 +132,70 @@ function twoLines(line1: string, line2: string) {
);
}
+function progress(done: number, awaiting: number): number {
+ const d = done + awaiting;
+ if (!d) return 0;
+ return done / d;
+}
+
+function capitalizeFirst(str: string): string {
+ return str.slice(0, 1).toUpperCase() + str.slice(1).toLowerCase();
+}
Review comment:
May be worth adding unit tests for all these new utility methods
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
