Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option to delete table and schema immediately #14736

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add option to delete table and schema immediately #14736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions pinot-controller/src/main/resources/app/components/Confirm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,11 @@ type Props = {
successCallback: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void;
closeDialog: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void;
dialogYesLabel?: string,
dialogNoLabel?: string
dialogNoLabel?: string,
children?: React.ReactNode
};

const Confirm = ({openDialog, dialogTitle, dialogContent, successCallback, closeDialog, dialogYesLabel = 'Yes', dialogNoLabel = 'No'}: Props) => {
const Confirm = ({openDialog, dialogTitle, dialogContent, successCallback, closeDialog, dialogYesLabel = 'Yes', dialogNoLabel = 'No', children}: Props) => {
const classes = useStyles();
const [open, setOpen] = React.useState(openDialog);

Expand All @@ -89,6 +90,7 @@ const Confirm = ({openDialog, dialogTitle, dialogContent, successCallback, close
{dialogContent}
</DialogContentText>
: dialogContent}
{children}
</DialogContent>
<DialogActions style={{paddingBottom: 20}} className={`${isStringDialog ? classes.dialogActions : ''}`}>
<Button variant="outlined" onClick={closeDialog} color="secondary" className={classes.red}>
Expand Down
199 changes: 137 additions & 62 deletions pinot-controller/src/main/resources/app/pages/TenantDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import React, { useState, useEffect, useRef } from 'react';
import { makeStyles } from '@material-ui/core/styles';
import { Box, Button, FormControlLabel, Grid, Switch, Tooltip, Typography } from '@material-ui/core';
import { Box, Button, Checkbox, FormControlLabel, Grid, Switch, Tooltip, Typography } from '@material-ui/core';
import { RouteComponentProps, useHistory, useLocation } from 'react-router-dom';
import { UnControlled as CodeMirror } from 'react-codemirror2';
import { DISPLAY_SEGMENT_STATUS, InstanceState, TableData, TableSegmentJobs, TableType } from 'Models';
Expand Down Expand Up @@ -152,6 +152,18 @@ const TenantPageDetails = ({ match }: RouteComponentProps<Props>) => {
const [showRebalanceServerModal, setShowRebalanceServerModal] = useState(false);
const [schemaJSONFormat, setSchemaJSONFormat] = useState(false);

// This is quite hacky, but it's the only way to get this to work with the dialog.
// The useState variables are simply for the dialog box to know what to render in
// the checkbox fields. The actual state of the checkboxes is stored in the refs.
// The refs are then used to determine how we delete the table. If you try to use
// the state variables in this class, you will not be able to get their latest values.
const [dialogCheckboxes, setDialogCheckboxes] = useState({
deleteImmediately: false,
deleteSchema: false
});
const deleteImmediatelyRef = useRef(false);
const deleteSchemaRef = useRef(false);

Comment on lines +155 to +166
Copy link
Contributor

Choose a reason for hiding this comment

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

if the state can be used to render checkbox value then it can be used at other places as well.
do you know the reason of this behaviour? Iet me know if you want me to debug. I think its some react rendering issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know react well enough, but I spent days on this with a coworker as well as multiple LLM tools, and this is the only thing that reliably worked. I think the issue is the very strange lifecycle of state variables and functions.

  • "show dialog" state variable is in TenantDetails
  • checkbox definition is in TenantDetails
  • checkbox state is in TenantDetails
  • the actual modal is in Confirm
  • the children are rendered in Confirm
  • the callbacks are defined in TenantDetails but activated in Confirm

Reusing the state variables from TenantDetails and rendering them in Confirm, I only got 2 bad things working

  • the checkbox updates, but the callback doesn't see the latest value until the second time it's called
  • the callback sees the latest value, but the checkbox doesn't actually visually update

This leads me to believe that the way we're creating + rendering the modal does not allow for a single state variable to work since there's no possible update order for every component to do the right thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thanks for the info. Let me try debugging.
Will update you here in a day or two. if not this solution looks good enough.

const fetchTableData = async () => {
// We keep all the fetching inside this component since we need to be able
// to handle users making changes to the table and then reloading the json.
Expand Down Expand Up @@ -299,45 +311,66 @@ const TenantPageDetails = ({ match }: RouteComponentProps<Props>) => {
};

const handleDeleteTableAction = () => {
// Set checkboxes to the last state when opening dialog
setDialogCheckboxes({
deleteImmediately: deleteImmediatelyRef.current,
deleteSchema: deleteSchemaRef.current
});

setDialogDetails({
title: 'Delete Table',
content: 'Are you sure want to delete this table? All data and configs will be deleted.',
successCb: () => deleteTable()
content:
'Are you sure want to delete this table? All data and configs will be deleted.',
successCb: () => {
deleteTable();
},
renderChildren: true,
});
setConfirmDialog(true);
};

const deleteTable = async () => {
const result = await PinotMethodUtils.deleteTableOp(tableName);
if(result.status){
dispatch({type: 'success', message: result.status, show: true});
} else {
dispatch({type: 'error', message: result.error, show: true});
}
closeDialog();
if(result.status){
setTimeout(()=>{
if(tenantName){
history.push(Utils.navigateToPreviousPage(location, true));
} else {
history.push('/tables');
}
}, 1000);
}
};
let message = '';
let tableDeleted = false;
let schemaDeleted = false;

const handleDeleteSchemaAction = () => {
setDialogDetails({
title: 'Delete Schema',
content: 'Are you sure want to delete this schema? Any tables using this schema might not function correctly.',
successCb: () => deleteSchema()
});
setConfirmDialog(true);
};
try {
const tableRes = await PinotMethodUtils.deleteTableOp(tableName, deleteImmediatelyRef.current ? '0d' : undefined);

if (tableRes.status) {
message = tableRes.status;
tableDeleted = true;

if (deleteSchemaRef.current) {
const schemaRes = await PinotMethodUtils.deleteSchemaOp(schemaJSON.schemaName);
if (schemaRes.status) {
message += ". " + schemaRes.status;
schemaDeleted = true;
} else {
message += ". " + schemaRes.error || "Unknown error deleting schema";
}
}
} else {
message = tableRes.error || "Unknown error deleting table";
}

const deleteSchema = async () => {
const result = await PinotMethodUtils.deleteSchemaOp(schemaJSON.schemaName);
syncResponse(result);
const isSuccess = tableDeleted && (!deleteSchemaRef.current || schemaDeleted);
dispatch({ type: isSuccess ? 'success' : 'error', message, show: true });
closeDialog();

if (tableDeleted) {
setTimeout(() => {
if (tenantName) {
history.push(Utils.navigateToPreviousPage(location, true));
} else {
history.push('/tables');
}
}, 1000);
}
} catch (error) {
dispatch({ type: 'error', message: error.toString(), show: true });
closeDialog();
}
};

const handleReloadSegments = () => {
Expand Down Expand Up @@ -479,20 +512,6 @@ const TenantPageDetails = ({ match }: RouteComponentProps<Props>) => {
>
Edit Schema
</CustomButton>
<CustomButton
isDisabled={!schemaJSON} onClick={handleDeleteSchemaAction}
tooltipTitle="Delete Schema"
enableTooltip={true}
>
Delete Schema
</CustomButton>
<CustomButton
isDisabled={true} onClick={()=>{}}
tooltipTitle="Truncate Table"
enableTooltip={true}
>
Truncate Table
</CustomButton>
<CustomButton
onClick={handleReloadSegments}
tooltipTitle="Reloads all segments of the table to apply changes such as indexing, column default values, etc"
Expand Down Expand Up @@ -647,35 +666,91 @@ const TenantPageDetails = ({ match }: RouteComponentProps<Props>) => {
</Grid>
<EditConfigOp
showModal={showEditConfig}
hideModal={()=>{setShowEditConfig(false);}}
hideModal={() => {
setShowEditConfig(false);
}}
saveConfig={saveConfigAction}
config={config}
handleConfigChange={handleConfigChange}
/>
{
showReloadStatusModal &&
{showReloadStatusModal && (
<ReloadStatusOp
hideModal={()=>{setShowReloadStatusModal(false); setReloadStatusData(null)}}
hideModal={() => {
setShowReloadStatusModal(false);
setReloadStatusData(null);
}}
reloadStatusData={reloadStatusData}
tableJobsData={tableJobsData}
/>
}
{showRebalanceServerModal &&
)}
{showRebalanceServerModal && (
<RebalanceServerTableOp
hideModal={()=>{setShowRebalanceServerModal(false)}}
hideModal={() => {
setShowRebalanceServerModal(false);
}}
tableType={tableType.toUpperCase()}
tableName={tableName}
/>
}
{confirmDialog && dialogDetails && <Confirm
openDialog={confirmDialog}
dialogTitle={dialogDetails.title}
dialogContent={dialogDetails.content}
successCallback={dialogDetails.successCb}
closeDialog={closeDialog}
dialogYesLabel='Yes'
dialogNoLabel='No'
/>}
)}
{confirmDialog && dialogDetails && (
<Confirm
openDialog={confirmDialog}
dialogTitle={dialogDetails.title}
dialogContent={dialogDetails.content}
successCallback={dialogDetails.successCb}
children={
dialogDetails.renderChildren && <>
<Tooltip
title="Delete table and all segments immediately.
If not checked, all segments will be copied over to a separate path
and kept until the cluster default retention period is met."
arrow
placement="right"
>
<FormControlLabel
control={
<Checkbox
checked={dialogCheckboxes.deleteImmediately}
onChange={(e) => {
const { checked } = e.target;
setDialogCheckboxes(prev => ({
...prev,
deleteImmediately: checked
}));
deleteImmediatelyRef.current = checked
}}
color="primary"
/>
}
label="Delete Immediately"
/>
</Tooltip>
<Tooltip title="If the table is successfully deleted, also delete the schema associated with this table." arrow placement="right">
<FormControlLabel
control={
<Checkbox
checked={dialogCheckboxes.deleteSchema}
onChange={(e) => {
const { checked } = e.target;
setDialogCheckboxes(prev => ({
...prev,
deleteSchema: checked
}));
deleteSchemaRef.current = checked;
}}
color="primary"
/>
}
label="Delete Schema"
/>
</Tooltip>
</>
}
closeDialog={closeDialog}
dialogYesLabel="Yes"
dialogNoLabel="No"
/>
)}
</Grid>
);
}
Expand Down
4 changes: 2 additions & 2 deletions pinot-controller/src/main/resources/app/requests/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,8 @@ export const getTableJobs = (tableName: string, jobTypes?: string): Promise<Axio
export const getSegmentReloadStatus = (jobId: string): Promise<AxiosResponse<OperationResponse>> =>
baseApi.get(`/segments/segmentReloadStatus/${jobId}`, {headers});

export const deleteTable = (tableName: string): Promise<AxiosResponse<OperationResponse>> =>
baseApi.delete(`/tables/${tableName}`, {headers});
export const deleteTable = (tableName: string, retention?: string): Promise<AxiosResponse<OperationResponse>> =>
baseApi.delete(`/tables/${tableName}${retention ? `?retention=${retention}` : ''}`, {headers});

export const deleteSchema = (schemaName: string): Promise<AxiosResponse<OperationResponse>> =>
baseApi.delete(`/schemas/${schemaName}`, {headers});
Expand Down
4 changes: 2 additions & 2 deletions pinot-controller/src/main/resources/app/utils/PinotMethodUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,8 +970,8 @@ const updateSchema = (schemaName: string, schema: string, reload?: boolean) => {
})
};

const deleteTableOp = (tableName) => {
return deleteTable(tableName).then((response)=>{
const deleteTableOp = (tableName: string, retention?: string) => {
return deleteTable(tableName, retention).then((response)=>{
return response.data;
});
};
Expand Down
Loading