Skip to content

Web console: Display compaction status#10438

Merged
vogievetsky merged 5 commits intoapache:masterfrom
implydata:compaction-feedback
Sep 29, 2020
Merged

Web console: Display compaction status#10438
vogievetsky merged 5 commits intoapache:masterfrom
implydata:compaction-feedback

Conversation

@vogievetsky
Copy link
Copy Markdown
Contributor

This PR adds a column to display the (new) compaction status API

image

This PR also:

  • Adds suggestion options for the skipOffsetFromLatest parameter
  • Improves error cursor placement in the JsonInput
  • adds an alt + click option to the ... button allowing for a secret undocumented API option to run compaction immediately. This is very useful for debugging and the copy is made suitably scary to ensure users are dissuade from using this option.

setInternalValue({
value,
stringified: stringifyJson(value),
});
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no logical change here... just wanted to have less nesting

export interface MoreButtonProps {
children: React.ReactNode;
children: React.ReactNode | React.ReactNode[];
altExtra?: React.ReactNode;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding another snapshot test that sets the new altExtra prop?

Comment on lines +234 to +236
export function formatPercent(n: number): string {
return (n * 100).toFixed(2) + '%';
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing unit test in general.spec.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +135 to +143
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();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth adding unit tests for all these new utility methods

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +642 to +658
<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>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am 👍 to modify the e2e test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a stab at modifying the autocompaction E2E test to use the new trigger compaction UI: #10469

Comment on lines +396 to +405
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,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. #9956 used this approach to test SegmentTimeline with a mock for QueryManager.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testing refactor I suggested, could be fairly involved as it'd be good to add tests for the parts of the datasources view that are not being modified by this PR. Perhaps it'd be best to have a separate PR that has tests for all the parts.

@vogievetsky
Copy link
Copy Markdown
Contributor Author

I think this is good to go, and some of the tests that were addressed in review should be added in a subsequent PR

@vogievetsky
Copy link
Copy Markdown
Contributor Author

thank you for the feedback! Will followup a PR with fancier e2e tests

@vogievetsky vogievetsky merged commit 729bcba into apache:master Sep 29, 2020
@vogievetsky vogievetsky deleted the compaction-feedback branch September 29, 2020 05:19
@jon-wei jon-wei added this to the 0.20.0 milestone Sep 30, 2020
ccaominh added a commit to ccaominh/druid that referenced this pull request Oct 1, 2020
Restore the web console's ability to view a datasource's compaction
configuration via the "action" menu. Refactoring done in
apache#10438 introduced a regression that
always caused the default compaction configuration to be shown via the
"action" menu instead.

Regression test is added in e2e-tests/auto-compaction.spec.ts.
vogievetsky pushed a commit that referenced this pull request Oct 2, 2020
Restore the web console's ability to view a datasource's compaction
configuration via the "action" menu. Refactoring done in
#10438 introduced a regression that
always caused the default compaction configuration to be shown via the
"action" menu instead.

Regression test is added in e2e-tests/auto-compaction.spec.ts.
ccaominh added a commit to ccaominh/druid that referenced this pull request Oct 2, 2020
Restore the web console's ability to view a datasource's compaction
configuration via the "action" menu. Refactoring done in
apache#10438 introduced a regression that
always caused the default compaction configuration to be shown via the
"action" menu instead.

Regression test is added in e2e-tests/auto-compaction.spec.ts.
ccaominh added a commit that referenced this pull request Oct 2, 2020
)

* Web console reindexing E2E test (#10453)

Add an E2E test for the web console workflow of reindexing a Druid
datasource to change the secondary partitioning type.  The new test
changes dynamic to single dim partitions since the autocompaction test
already does dynamic to hashed partitions.

Also, run the web console E2E tests in parallel to reduce CI time and
change naming convention for test datasources to make it easier to map
them to the corresponding test run.

Main changes:

1) web-consolee2e-tests/reindexing.spec.ts
   - new E2E test

2) web-console/e2e-tests/component/load-data/data-connector/reindex.ts
   - new data loader connector for druid input source

3) web-console/e2e-tests/component/load-data/config/partition.ts
   - move partition spec definitions from compaction.ts
   - add new single dim partition spec definition

* Fix UI datasources view edit action compaction (#10459)

Restore the web console's ability to view a datasource's compaction
configuration via the "action" menu. Refactoring done in
#10438 introduced a regression that
always caused the default compaction configuration to be shown via the
"action" menu instead.

Regression test is added in e2e-tests/auto-compaction.spec.ts.
JulianJaffePinterest pushed a commit to JulianJaffePinterest/druid that referenced this pull request Jan 22, 2021
* init compaction status

* % compacted

* final UI tweaks

* extracted utils, added tests

* add tests to general foramt functions
JulianJaffePinterest pushed a commit to JulianJaffePinterest/druid that referenced this pull request Jan 22, 2021
Restore the web console's ability to view a datasource's compaction
configuration via the "action" menu. Refactoring done in
apache#10438 introduced a regression that
always caused the default compaction configuration to be shown via the
"action" menu instead.

Regression test is added in e2e-tests/auto-compaction.spec.ts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants