Skip to content

Cb 4270 row counter add cancel button for big tables#2545

Merged
Wroud merged 41 commits intodevelfrom
CB-4270-row-counter-add-cancel-button-for-big-tables
Apr 23, 2024
Merged

Cb 4270 row counter add cancel button for big tables#2545
Wroud merged 41 commits intodevelfrom
CB-4270-row-counter-add-cancel-button-for-big-tables

Conversation

@sergeyteleshev
Copy link
Copy Markdown
Contributor

No description provided.

@sergeyteleshev sergeyteleshev self-assigned this Apr 11, 2024
Comment thread webapp/packages/plugin-data-viewer/src/DatabaseDataModel/IDatabaseDataSource.ts Outdated
Comment thread webapp/packages/plugin-data-viewer/src/locales/zh.ts Outdated
Comment thread webapp/packages/core-blocks/src/ToolsPanel/ToolsAction.tsx Outdated
Comment thread webapp/packages/plugin-data-viewer/src/DataViewerTableService.ts Outdated
Comment thread webapp/packages/plugin-data-viewer/src/DatabaseDataModel/DatabaseDataSource.ts Outdated
Comment thread webapp/packages/plugin-data-viewer/src/ResultSetDataSource.ts Outdated
Comment thread webapp/packages/plugin-data-viewer/src/ResultSetDataSource.ts Outdated

// TODO: we need to dispose table model, but don't close execution context, so now we only
const model = this.tableViewerStorageService.get(group.modelId);
model?.source.cancelLoadTotalCount();
Copy link
Copy Markdown
Contributor Author

@sergeyteleshev sergeyteleshev Apr 12, 2024

Choose a reason for hiding this comment

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

since there is no dispose, we need to cancel it in result set tab manually on close

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

then it's a bug cuz we need to call dispose for each unused table model
but in SQL editor we have special behavior where we can reuse the same model for different queries so check it please

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you can move cancelLoadTotalCount call to cancel fn

const cancelled = model.source.cancelLoadTotalCountTask?.cancelled;

if (!cancelled) {
notificationService.logException(e, 'data_viewer_total_count_cancel_failed_title', typeof e === 'string' ? e : undefined);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why do we need typeof e === 'string' ? e : undefined here?

return <TotalCountAction loading={loading} resultIndex={resultIndex} model={model} onClick={loadTotalCount} />;
});

const CancelTotalCountAction = observer(function CancelTotalCountAction({ onClick }: { onClick: VoidFunction }) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please do not declare multiple components in one file

['data_viewer_refresh_result_set', 'Refresh result set'],
['data_viewer_total_count_tooltip', 'Get total count'],
['data_viewer_total_count_failed', 'Failed to get total count'],
['data_viewer_total_count_cancel_failed_title', 'Failed to cancel getting of total count'],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure that we should create new message token as "Task was cancelled" is throw by the task itself

@sergeyteleshev sergeyteleshev requested a review from Wroud April 15, 2024 14:44
Comment thread webapp/packages/plugin-data-viewer/src/ContainerDataSource.ts Outdated
Comment thread webapp/packages/plugin-data-viewer/src/ContainerDataSource.ts Outdated
Comment thread webapp/packages/plugin-data-viewer/src/ResultSetDataSource.ts Outdated
@sergeyteleshev sergeyteleshev requested a review from Wroud April 15, 2024 18:27
Comment thread webapp/packages/plugin-data-viewer/src/DatabaseDataModel/IDatabaseDataSource.ts Outdated

async closeResults(results: IDatabaseResultSet[]): Promise<void> {
await this.connectionExecutionContextService.load();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

await this.connectionExecutionContextService.load();
can be removed

Comment on lines +169 to +170
model?.source.closeResults?.(model.getResults());
model?.cancel();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

model.source.closeResults(model.getResults());
model.cancel();

@sergeyteleshev sergeyteleshev requested a review from Wroud April 16, 2024 07:34
@yagudin10 yagudin10 force-pushed the CB-4270-row-counter-add-cancel-button-for-big-tables branch from 65ef929 to d894010 Compare April 18, 2024 13:28
<span className={styles.cancelText}>{translate('ui_processing_cancel')}</span>
<Container
className={s(style, { action: true })}
title={loading ? translate('ui_processing_canceling') : translate('ui_processing_cancel')}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

translate(loading ? 'ui_processing_canceling' : 'ui_processing_cancel')

if (this.currentTask) {
await this.currentTask.cancel();
}
await Promise.all([this.currentTask?.cancel(), super.cancel()]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

await super.cancel();
await this.currentTask?.cancel()

Comment on lines +24 to +26
async cancel(): Promise<void> {
await Promise.all([this.cancelLoadTotalCount(), super.cancel()]);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

await super.cancel();
await this.cancelLoadTotalCount();

loading: boolean;
}

export const CancelTotalCountAction = observer(function CancelTotalCountAction({ onClick, loading }: Props) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please use observer<Props>

if (this.currentTask) {
await this.currentTask.cancel();
}
await Promise.all([this.currentTask?.cancel(), super.cancel()]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

await super.cancel();
await this.currentTask?.cancel();

@Wroud Wroud merged commit 51ef899 into devel Apr 23, 2024
@Wroud Wroud deleted the CB-4270-row-counter-add-cancel-button-for-big-tables branch April 23, 2024 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants