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.

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

fix: cache icon fro map task #519

Merged
merged 6 commits into from
Jun 28, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ export const NodeExecutionDetailsPanelContent: React.FC<NodeExecutionDetailsProp
if (nodeExecution) {
detailsContent = (
<>
<NodeExecutionCacheStatus taskNodeMetadata={nodeExecution.closure.taskNodeMetadata} />
<NodeExecutionCacheStatus execution={nodeExecution} />
<ExecutionTypeDetails details={details} execution={nodeExecution} />
</>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { timestampToDate } from 'common/utils';
import { CatalogCacheStatus, NodeExecutionPhase } from 'models/Execution/enums';
import { dNode } from 'models/Graph/types';
import { TaskType } from 'models/Task/constants';
import { BarItemData } from './utils';

const WEEK_DURATION_SEC = 7 * 24 * 3600;
Expand All @@ -10,6 +11,7 @@ const EMPTY_BAR_ITEM: BarItemData = {
startOffsetSec: 0,
durationSec: 0,
isFromCache: false,
isMapTaskCache: false,
};

export const getChartDurationData = (
Expand All @@ -20,7 +22,8 @@ export const getChartDurationData = (

let totalDurationSec = 0;
const initialStartTime = startedAt.getTime();
const result: BarItemData[] = nodes.map(({ execution }) => {

const result: BarItemData[] = nodes.map(({ execution, value }) => {
if (!execution) {
return EMPTY_BAR_ITEM;
}
Expand All @@ -29,6 +32,9 @@ export const getChartDurationData = (
const isFromCache =
execution.closure.taskNodeMetadata?.cacheStatus === CatalogCacheStatus.CACHE_HIT;

const isMapTaskCache =
value?.template?.type === TaskType.ARRAY && value?.template?.metadata?.cacheSerializable;

// Offset values
let startOffset = 0;
const startedAt = execution.closure.startedAt;
Expand Down Expand Up @@ -67,7 +73,7 @@ export const getChartDurationData = (

const startOffsetSec = Math.trunc(startOffset / 1000);
totalDurationSec = Math.max(totalDurationSec, startOffsetSec + durationSec);
return { phase, startOffsetSec, durationSec, isFromCache };
return { phase, startOffsetSec, durationSec, isFromCache, isMapTaskCache };
});

// Do we want to get initialStartTime from different place, to avoid recalculating it.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getNodeExecutionPhaseConstants } from 'components/Executions/utils';
import { primaryTextColor } from 'components/Theme/constants';
import { NodeExecutionPhase } from 'models/Execution/enums';
import t from 'components/Executions/strings';

export const CASHED_GREEN = 'rgba(74,227,174,0.25)'; // statusColors.SUCCESS (Mint20) with 25% opacity
export const TRANSPARENT = 'rgba(0, 0, 0, 0)';
Expand All @@ -10,6 +11,7 @@ export interface BarItemData {
startOffsetSec: number;
durationSec: number;
isFromCache: boolean;
isMapTaskCache: boolean;
}

interface ChartDataInput {
Expand Down Expand Up @@ -60,11 +62,24 @@ export const generateChartData = (data: BarItemData[]): ChartDataInput => {
// don't show Label if there is now duration yet.
const labelString = element.durationSec > 0 ? durationString : '';

const generateTooltipLabelText = (element: BarItemData): string[] => {
if (element.isMapTaskCache) return [tooltipString, t('mapCacheMessage')];
if (element.isFromCache) return [tooltipString, t('readFromCache')];

return [tooltipString];
};

const generateBarLabelText = (element: BarItemData): string => {
if (element.isMapTaskCache) return '\u229A ' + t('mapCacheMessage');
if (element.isFromCache) return '\u229A ' + t('fromCache');
return labelString;
};

durations.push(element.durationSec);
startOffset.push(element.startOffsetSec);
offsetColor.push(element.isFromCache ? CASHED_GREEN : TRANSPARENT);
tooltipLabel.push(element.isFromCache ? [tooltipString, 'Read from cache'] : [tooltipString]);
barLabel.push(element.isFromCache ? '\u229A From cache' : labelString);
tooltipLabel.push(generateTooltipLabelText(element));
barLabel.push(generateBarLabelText(element));
barColor.push(phaseConstant.badgeColor);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,21 @@ describe('ExecutionDetails > Timeline > BarChart', () => {

it('generateChartData properly generates map of data for ChartBars', () => {
const chartData = generateChartData(mockbarItems);
expect(chartData.durations).toEqual([15, 11, 23, 0]);
expect(chartData.startOffset).toEqual([0, 5, 17, 39]);
expect(chartData.offsetColor).toEqual([TRANSPARENT, CASHED_GREEN, TRANSPARENT, TRANSPARENT]);
expect(chartData.durations).toEqual([15, 11, 23, 0, 11]);
expect(chartData.startOffset).toEqual([0, 5, 17, 39, 5]);
expect(chartData.offsetColor).toEqual([
TRANSPARENT,
CASHED_GREEN,
TRANSPARENT,
TRANSPARENT,
TRANSPARENT,
]);
// labels looks as expected
expect(chartData.barLabel[0]).toEqual(formatSecondsToHmsFormat(mockbarItems[0].durationSec));
expect(chartData.barLabel[1]).toEqual('\u229A From cache');
expect(chartData.barLabel[3]).toEqual('');
expect(chartData.barLabel[4]).toEqual(
"\u229A Check the detail panel for each task's cache status.",
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,41 @@ const getMockNodeExecution = (
};

export const mockbarItems = [
{ phase: NodeExecutionPhase.FAILED, startOffsetSec: 0, durationSec: 15, isFromCache: false },
{ phase: NodeExecutionPhase.SUCCEEDED, startOffsetSec: 5, durationSec: 11, isFromCache: true },
{ phase: NodeExecutionPhase.RUNNING, startOffsetSec: 17, durationSec: 23, isFromCache: false },
{ phase: NodeExecutionPhase.QUEUED, startOffsetSec: 39, durationSec: 0, isFromCache: false },
{
phase: NodeExecutionPhase.FAILED,
startOffsetSec: 0,
durationSec: 15,
isFromCache: false,
isMapTaskCache: false,
},
{
phase: NodeExecutionPhase.SUCCEEDED,
startOffsetSec: 5,
durationSec: 11,
isFromCache: true,
isMapTaskCache: false,
},
{
phase: NodeExecutionPhase.RUNNING,
startOffsetSec: 17,
durationSec: 23,
isFromCache: false,
isMapTaskCache: false,
},
{
phase: NodeExecutionPhase.QUEUED,
startOffsetSec: 39,
durationSec: 0,
isFromCache: false,
isMapTaskCache: false,
},
{
phase: NodeExecutionPhase.SUCCEEDED,
startOffsetSec: 5,
durationSec: 11,
isFromCache: false,
isMapTaskCache: true,
},
];

export const getMockExecutionsForBarChart = (startTimeSec: number) => {
Expand All @@ -73,5 +104,6 @@ export const getMockExecutionsForBarChart = (startTimeSec: number) => {
getMockNodeExecution(start, NodeExecutionPhase.SUCCEEDED, 5, 11, CatalogCacheStatus.CACHE_HIT),
getMockNodeExecution(start, NodeExecutionPhase.RUNNING, 17, 23, CatalogCacheStatus.CACHE_MISS),
getMockNodeExecution(start, NodeExecutionPhase.QUEUED, 39, 0),
getMockNodeExecution(start, NodeExecutionPhase.SUCCEEDED, 5, 11, CatalogCacheStatus.MAP_CACHE),
];
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@ import classnames from 'classnames';
import { assertNever } from 'common/utils';
import { PublishedWithChangesOutlined } from 'components/common/PublishedWithChanges';
import { useCommonStyles } from 'components/common/styles';
import { NodeExecutionDetails } from 'components/Executions/types';
import { useNodeExecutionContext } from 'components/Executions/contextProvider/NodeExecutionDetails';
import { CatalogCacheStatus } from 'models/Execution/enums';
import { TaskNodeMetadata } from 'models/Execution/types';
import { NodeExecution, TaskExecutionIdentifier } from 'models/Execution/types';
import { TaskType } from 'models/Task/constants';
import * as React from 'react';
import { Link as RouterLink } from 'react-router-dom';
import { Routes } from 'routes/routes';
Expand Down Expand Up @@ -50,6 +53,9 @@ export const NodeExecutionCacheStatusIcon: React.FC<
case CatalogCacheStatus.CACHE_PUT_FAILURE: {
return <ErrorOutlined {...props} ref={ref} />;
}
case CatalogCacheStatus.MAP_CACHE: {
return <InfoOutlined {...props} ref={ref} />;
}
default: {
assertNever(status);
return null;
Expand All @@ -58,7 +64,7 @@ export const NodeExecutionCacheStatusIcon: React.FC<
});

export interface NodeExecutionCacheStatusProps {
taskNodeMetadata?: TaskNodeMetadata;
execution: NodeExecution;
/** `normal` will render an icon with description message beside it
* `iconOnly` will render just the icon with the description as a tooltip
*/
Expand All @@ -68,20 +74,78 @@ export interface NodeExecutionCacheStatusProps {
* the cache status with a descriptive message. For `Core.CacheCatalogStatus.CACHE_HIT`,
* it will also attempt to render a link to the source `WorkflowExecution` (normal
* variant only).
*
* For Map Tasks, we will check the NodeExecutionDetail for the cache status instead. Since map tasks
* cotains multiple tasks, the logic of the cache status is different.
*/
export const NodeExecutionCacheStatus: React.FC<NodeExecutionCacheStatusProps> = ({
taskNodeMetadata,
execution,
variant = 'normal',
}) => {
const taskNodeMetadata = execution.closure?.taskNodeMetadata;
const detailsContext = useNodeExecutionContext();
const [nodeDetails, setNodeDetails] = React.useState<NodeExecutionDetails | undefined>();

React.useEffect(() => {
let isCurrent = true;
detailsContext.getNodeExecutionDetails(execution).then((res) => {
if (isCurrent) {
setNodeDetails(res);
}
});
return () => {
isCurrent = false;
};
});

console.log('testiung', nodeDetails);
anrusina marked this conversation as resolved.
Show resolved Hide resolved
if (nodeDetails?.taskTemplate?.type === TaskType.ARRAY) {
anrusina marked this conversation as resolved.
Show resolved Hide resolved
if (nodeDetails?.taskTemplate?.metadata?.cacheSerializable) {
return <CacheStatus cacheStatus={CatalogCacheStatus.MAP_CACHE} variant={variant} />;
}
}

if (taskNodeMetadata == null || taskNodeMetadata.cacheStatus == null) {
anrusina marked this conversation as resolved.
Show resolved Hide resolved
return null;
}

const sourceTaskExecution = taskNodeMetadata.catalogKey?.sourceTaskExecution;

return (
<CacheStatus
cacheStatus={taskNodeMetadata.cacheStatus}
sourceTaskExecution={sourceTaskExecution}
variant={variant}
/>
);
};

export interface CacheStatusProps {
cacheStatus: CatalogCacheStatus | null | undefined;
/** `normal` will render an icon with description message beside it
* `iconOnly` will render just the icon with the description as a tooltip
*/
variant?: 'normal' | 'iconOnly';
sourceTaskExecution?: TaskExecutionIdentifier;
iconStyles?: React.CSSProperties;
}

export const CacheStatus: React.FC<CacheStatusProps> = ({
cacheStatus,
sourceTaskExecution,
variant = 'normal',
iconStyles,
}) => {
const commonStyles = useCommonStyles();
const styles = useStyles();
if (taskNodeMetadata == null || taskNodeMetadata.cacheStatus == null) {

if (cacheStatus == null) {
return null;
}

const message = cacheStatusMessages[taskNodeMetadata.cacheStatus] || unknownCacheStatusString;
const message = cacheStatusMessages[cacheStatus] || unknownCacheStatusString;

const sourceExecutionId = taskNodeMetadata.catalogKey?.sourceTaskExecution;
const sourceExecutionId = sourceTaskExecution;
const sourceExecutionLink = sourceExecutionId ? (
<RouterLink
className={classnames(commonStyles.primaryLink, styles.sourceExecutionLink)}
Expand All @@ -99,14 +163,15 @@ export const NodeExecutionCacheStatus: React.FC<NodeExecutionCacheStatusProps> =
commonStyles.iconRight,
commonStyles.iconSecondary,
)}
status={taskNodeMetadata.cacheStatus}
style={iconStyles}
status={cacheStatus}
/>
</Tooltip>
) : (
<div>
<Typography className={styles.cacheStatus} variant="subtitle1" color="textSecondary">
<NodeExecutionCacheStatusIcon
status={taskNodeMetadata.cacheStatus}
status={cacheStatus}
className={classnames(commonStyles.iconSecondary, commonStyles.iconLeft)}
/>
{message}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,16 +139,13 @@ export function generateColumns(
label: 'type',
},
{
cellRenderer: ({
execution: {
closure: { phase = NodeExecutionPhase.UNDEFINED, taskNodeMetadata },
},
}) => (
cellRenderer: ({ execution }) => (
<>
<ExecutionStatusBadge phase={phase} type="node" />
{hasCacheStatus(taskNodeMetadata) ? (
<NodeExecutionCacheStatus taskNodeMetadata={taskNodeMetadata} variant="iconOnly" />
) : null}
<ExecutionStatusBadge
phase={execution.closure?.phase || NodeExecutionPhase.UNDEFINED}
anrusina marked this conversation as resolved.
Show resolved Hide resolved
type="node"
/>
<NodeExecutionCacheStatus execution={execution} variant="iconOnly" />
</>
),
className: styles.columnStatus,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,8 @@ describe('NodeExecutionsTable', () => {
CatalogCacheStatus.CACHE_LOOKUP_FAILURE,
CatalogCacheStatus.CACHE_POPULATED,
CatalogCacheStatus.CACHE_PUT_FAILURE,
CatalogCacheStatus.CACHE_MISS,
CatalogCacheStatus.CACHE_DISABLED,
].forEach((cacheStatusValue) =>
it(`renders correct icon for ${CatalogCacheStatus[cacheStatusValue]}`, async () => {
taskNodeMetadata.cacheStatus = cacheStatusValue;
Expand All @@ -254,19 +256,6 @@ describe('NodeExecutionsTable', () => {
);
}),
);

[CatalogCacheStatus.CACHE_DISABLED, CatalogCacheStatus.CACHE_MISS].forEach(
(cacheStatusValue) =>
it(`renders no icon for ${CatalogCacheStatus[cacheStatusValue]}`, async () => {
taskNodeMetadata.cacheStatus = cacheStatusValue;
updateNodeExecutions([cachedNodeExecution]);
const { getByText, queryByTitle } = renderTable();
await waitFor(() => {
getByText(cachedNodeExecution.id.nodeId);
});
expect(queryByTitle(cacheStatusMessages[cacheStatusValue])).toBeNull();
}),
);
});
});

Expand Down
Loading