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

Jump to bottom

DCOS-51596: Services Table: avoid rerenders #3792

Closed
wants to merge 2 commits into from
Closed

DCOS-51596: Services Table: avoid rerenders #3792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fbd4cc9
perf(services): avoid rerenders for service status table column
Apr 8, 2019
91b464d
perf: avoid service table re-renders if data unchanged
mperrotti Apr 9, 2019
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
123 changes: 96 additions & 27 deletions plugins/services/src/js/columns/ServicesTableStatusColumn.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import Pod from "../structs/Pod";
import Service from "../structs/Service";
import ServiceTree from "../structs/ServiceTree";
import ServiceTableUtil from "../utils/ServiceTableUtil";
import { getStatusIconProps } from "../utils/ServiceStatusIconUtil";

function statusCountsToTooltipContent(counts: {
total: number;
Expand All @@ -29,67 +30,135 @@ function statusCountsToTooltipContent(counts: {
});
}

export function statusRenderer(node: Service | ServiceTree): React.ReactNode {
return node instanceof ServiceTree
? renderServiceTree(node)
: renderService(node);
//Incoming
interface StatusColumnProps {
statusText: string;
instancesCount: number;
runningInstances: number;
timeWaiting: string | null;
timeQueued: number | null;
serviceStatus: object;
isServiceTree: boolean;
isService: boolean;
id: string | number;
displayDeclinedOffers: boolean;
appsWithWarnings: number | null;
unableToLaunch: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

these props look like they could benefit from telling apart a couple of things.
i'd propose to start with services and groups (a.k.a serviceTree here). there seems to be uncertainty (| null) by not telling them apart. also it looks like isServiceTree and isService is the same information because it's either one or another. that seems to cry for a union type.

}

function renderService(service: Service | Pod): React.ReactNode {
const status = service.getStatus();
if (isNA(status)) {
interface TreeStatusColumnProps extends StatusColumnProps {
statusText: string;
totalCount: number;
priorityStatusCount: number;
tooltipContent: JSX.Element;
}

export function statusRenderer(
service: Service | ServiceTree
): React.ReactNode {
const iconProps = getStatusIconProps(service);

const props = {
statusText: service.getStatus(),
instancesCount: service.getInstancesCount(),
runningInstances: service.getRunningInstancesCount()
};
let serviceTreeProps = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move that into a block where we already know that service is a ServiceTree and make it a const? this would get rid of having to ask service instanceof ServiceTree multiple times here.

if (service instanceof ServiceTree) {
const summary = service.getServiceTreeStatusSummary();
const statusText = ServiceStatus.toCategoryLabel(summary.status);
const totalCount = summary.counts.total;
const priorityStatusCount = summary.counts.status[summary.status];
const tooltipContent = statusCountsToTooltipContent(summary.counts);
serviceTreeProps = {
statusText,
totalCount,
priorityStatusCount,
tooltipContent
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if the code would tell a clearer story if we moved those props right into RenderServiceTree (currently line 2).

};
}

return service instanceof ServiceTree ? (
<RenderServiceTree {...iconProps} {...props} {...serviceTreeProps} />
) : (
<RenderService {...iconProps} {...props} />
);
}

//Master
Copy link
Contributor

Choose a reason for hiding this comment

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

there are multiple comments still in here.

function RenderService(props: StatusColumnProps): React.ReactNode {
//const status = service.getStatus();
if (isNA(props.statusText)) {
return null;
}
const instancesCount = service.getInstancesCount();
//const instancesCount = service.getInstancesCount();

return (
<TextCell>
<div className="flex">
<div className="service-status-icon-wrapper">
<ServiceStatusIcon
service={service}
id={props.id}
isService={props.isService}
isServiceTree={props.isServiceTree}
serviceStatus={props.serviceStatus}
timeWaiting={props.timeWaiting}
timeQueued={props.timeQueued}
appsWithWarnings={props.appsWithWarnings}
displayDeclinedOffers={props.displayDeclinedOffers}
unableToLaunch={props.unableToLaunch}
showTooltip={true}
tooltipContent={
<Plural
value={service.getRunningInstancesCount()}
one={`# instance running out of ${instancesCount}`}
other={`# instances running out of ${instancesCount}`}
value={props.runningInstances}
one={`# instance running out of ${props.instancesCount}`}
other={`# instances running out of ${props.instancesCount}`}
/>
}
/>
<Trans id={status} render="span" className="status-bar-text" />
<Trans
id={props.statusText}
render="span"
className="status-bar-text"
/>
</div>
<div className="service-status-progressbar-wrapper">
<ServiceStatusProgressBar service={service} />
<ServiceStatusProgressBar
instancesCount={props.instancesCount}
serviceStatus={props.serviceStatus}
runningInstances={props.runningInstances}
/>
</div>
</div>
</TextCell>
);
}

function renderServiceTree(service: ServiceTree): React.ReactNode {
const summary = service.getServiceTreeStatusSummary();
const statusText = ServiceStatus.toCategoryLabel(summary.status);
if (isNA(statusText)) {
function RenderServiceTree(props: TreeStatusColumnProps): React.ReactNode {
if (isNA(props.statusText)) {
return null;
}
const totalCount = summary.counts.total;
const priorityStatusCount = summary.counts.status[summary.status];
return (
<TextCell>
<div className="service-status-icon-wrapper">
<ServiceStatusIcon
service={service}
id={props.id}
isService={props.isService}
isServiceTree={props.isServiceTree}
serviceStatus={props.serviceStatus}
timeWaiting={props.timeWaiting}
timeQueued={props.timeQueued}
appsWithWarnings={props.appsWithWarnings}
displayDeclinedOffers={props.displayDeclinedOffers}
unableToLaunch={props.unableToLaunch}
showTooltip={true}
tooltipContent={
<span>{statusCountsToTooltipContent(summary.counts)}</span>
}
tooltipContent={<span>{props.tooltipContent}</span>}
/>
<span className="status-bar-text">
<Trans id={statusText} />{" "}
{totalCount > 1 ? (
<Trans id={props.statusText} />{" "}
{props.totalCount > 1 ? (
<Trans>
({priorityStatusCount} of {totalCount})
({props.priorityStatusCount} of {props.totalCount})
</Trans>
) : null}
</span>
Expand Down
8 changes: 7 additions & 1 deletion plugins/services/src/js/components/ServiceBreadcrumbs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ class ServiceBreadcrumbs extends React.Component {
let iconDisplay = null;
const instancesCount = service.getInstancesCount();
const runningInstances = service.getRunningInstancesCount();
const serviceStatus = service.getServiceStatus();

const tooltipContent = (
<Plural
Expand All @@ -154,7 +155,12 @@ class ServiceBreadcrumbs extends React.Component {
if (this.props.taskID == null && this.props.params != null) {
progressBar = (
<BreadcrumbSupplementalContent hasProgressBar={true}>
<ServiceStatusProgressBar key="status-bar" service={service} />
<ServiceStatusProgressBar
key="status-bar"
instancesCount={instancesCount}
runningInstances={runningInstances}
serviceStatus={serviceStatus}
/>
</BreadcrumbSupplementalContent>
);
}
Expand Down
4 changes: 3 additions & 1 deletion plugins/services/src/js/components/ServiceList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import createReactClass from "create-react-class";
import { Link, routerShape } from "react-router";

import ServiceStatusIcon from "./ServiceStatusIcon";
import { getStatusIconProps } from "../utils/ServiceStatusIconUtil";

const ServiceList = createReactClass({
displayName: "ServiceList",
Expand Down Expand Up @@ -50,6 +51,7 @@ const ServiceList = createReactClass({
return services.map(service => {
const instancesCount = service.getInstancesCount();
const runningInstances = service.getRunningInstancesCount();
const iconProps = getStatusIconProps(service);

const tooltipContent = (
<Plural
Expand Down Expand Up @@ -81,9 +83,9 @@ const ServiceList = createReactClass({
content: (
<ServiceStatusIcon
key="icon"
service={service}
showTooltip={true}
tooltipContent={tooltipContent}
{...iconProps}
/>
),
tag: "div"
Expand Down
Loading