[GitHub] [druid] ccaominh commented on a change in pull request #10438: Web console: Display compaction status

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org