DCOS-51596: Services Table: avoid rerenders #3792

DanielMSchmidt · 2019-04-08T15:10:29Z

Testing

Open a services page with react dev tools and flash on rerender enabled and see that there are less then before this change
Check that loading icons and deploying status bars still display as before

Trade-offs

Dependencies

Screenshots

Before	After first commit	After - no render if no data change

natmegs · 2019-04-08T21:02:23Z

The status column conversion is in its own commit since it was more complex than the others and affected multiple files. Comparison of react updates:

Before: https://cl.ly/5fe7c276d2ff
After: https://cl.ly/9f77e02a8985

natmegs · 2019-04-09T19:44:07Z

Just finished a nasty rebase since the accurate service status stuff merged and touched a lot of the same files. Would be great if someone familiar with that work could give this a look over cc @TattdCodeMonkey @pierrebeitz @Poltergeist

brandonc

Brilliant and obviously a lot of work. 🎉 I saw about 1/3 or 1/2 as many updates as master branch

brandonc · 2019-04-09T20:53:39Z

I rebased in order to fix a commit message lint error.

brandonc · 2019-04-09T21:32:12Z

~~Fixing test failure~~ Natalie fixing test failures

DanielMSchmidt · 2019-04-10T08:42:07Z

Conflicts again and I run into strange bugs when I try to fix them (spent like 1,5h with it). I would say, let's get #3796 merged first and ship the status column individually.

DanielMSchmidt · 2019-04-10T08:52:14Z

I created a new issue to track the efforts for the status column: DCOS-51596

GeorgiSTodorov · 2019-04-11T07:32:20Z

When running with fixtures enabled, the services table is loading forever.

pierrebeitz

i think there are some gains in comprehensibility and this maintainability here that we should aim for.

also the services table does not load for me (for FE-cluster, SOAK and fixtures).

pierrebeitz · 2019-04-11T08:16:34Z

plugins/services/src/js/columns/ServicesTableStatusColumn.tsx

+  id: string | number;
+  displayDeclinedOffers: boolean;
+  appsWithWarnings: number | null;
+  unableToLaunch: boolean;


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.

pierrebeitz · 2019-04-11T08:17:50Z

plugins/services/src/js/columns/ServicesTableStatusColumn.tsx

+  );
+}
+
+//Master


there are multiple comments still in here.

pierrebeitz · 2019-04-11T08:19:16Z

plugins/services/src/js/columns/ServicesTableStatusColumn.tsx

+    instancesCount: service.getInstancesCount(),
+    runningInstances: service.getRunningInstancesCount()
+  };
+  let serviceTreeProps = {};


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.

pierrebeitz · 2019-04-11T08:20:37Z

plugins/services/src/js/columns/ServicesTableStatusColumn.tsx

+      statusText,
+      totalCount,
+      priorityStatusCount,
+      tooltipContent


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

pierrebeitz · 2019-04-11T08:23:48Z

plugins/services/src/js/components/ServiceStatusIcon.tsx

+  timeQueued: number | null;
+  appsWithWarnings: number | null;
+  displayDeclinedOffers: boolean;
+  unableToLaunch: boolean;


could we make it so that the statusIcon did not need to know about some of these things?
conceptually it feels like it should not need to know about whether it's associated to a service or a group/serviceTree for example.

pierrebeitz · 2019-04-11T09:05:50Z

plugins/services/src/js/utils/DeclinedOffersUtil.d.ts

+  since?: unknown;
+}
+
+interface ItemWithQueue {


that's neat!

pierrebeitz · 2019-04-11T09:06:33Z

plugins/services/src/js/utils/ServiceStatusIconUtil.ts

+  since: string | null;
+}
+
+const isUnableToLaunch = (service: Service | Pod | ServiceTree) => {


couldn't we use ItemWithQueue here instead of Service | Pod | ServiceTree?

pierrebeitz · 2019-04-11T09:06:58Z

plugins/services/src/js/utils/ServiceStatusIconUtil.ts

+}
+
+const isUnableToLaunch = (service: Service | Pod | ServiceTree) => {
+  const queue = service.getQueue() as ServiceQueue | null;


we wouldn't need the typecast here then 😊

pierrebeitz · 2019-04-11T09:08:05Z

plugins/services/src/js/utils/ServiceStatusIconUtil.ts

+
+const isUnableToLaunch = (service: Service | Pod | ServiceTree) => {
+  const queue = service.getQueue() as ServiceQueue | null;
+  const UNABLE_TO_LAUNCH_TIMEOUT = 1000 * 60 * 30;


we're recreating this constant every time we call isUnableToLaunch. let's pull this to the root-level of the file!

pierrebeitz · 2019-04-11T09:10:20Z

plugins/services/src/js/utils/ServiceStatusIconUtil.ts

+  const unableToLaunch = isUnableToLaunch(service);
+  const appsWithWarnings = isServiceTree
+    ? (service as ServiceTree)
+        .filterItems((item: any) => {


our typings say that this is a Service. why override it with any?

possibly the types of ServiceTree.filterItems are wrong? (i updated them and possibly forgot that a serviceTree can also contain another ServiceTree. if so, please let's fix the types instead of working around here.

nLight requested a review from mperrotti April 8, 2019 16:33

natmegs force-pushed the danielmschmidt/DCOS-51377 branch from 93a0d18 to 9f7ebc5 Compare April 8, 2019 20:56

DanielMSchmidt mentioned this pull request Apr 9, 2019

DCOS-51377: Services Table: avoid rerender (except for status icons) #3796

Merged

natmegs force-pushed the danielmschmidt/DCOS-51377 branch from d965536 to e0b3983 Compare April 9, 2019 19:42

natmegs marked this pull request as ready for review April 9, 2019 19:44

brandonc previously approved these changes Apr 9, 2019

View reviewed changes

brandonc dismissed their stale review via c545c25 April 9, 2019 20:51

brandonc force-pushed the danielmschmidt/DCOS-51377 branch 2 times, most recently from c545c25 to a9dca61 Compare April 9, 2019 20:53

brandonc previously approved these changes Apr 9, 2019

View reviewed changes

natmegs dismissed brandonc’s stale review via 21fcdbf April 9, 2019 22:01

natmegs force-pushed the danielmschmidt/DCOS-51377 branch from a9dca61 to 21fcdbf Compare April 9, 2019 22:01

DanielMSchmidt changed the title ~~DCOS-51377: Services Table: avoid rerenders~~ DCOS-51596: Services Table: avoid rerenders Apr 10, 2019

juliangieseke added this to the 1.13 milestone Apr 10, 2019

nLight removed this from the 1.13 milestone Apr 10, 2019

Daniel Schmidt and others added 2 commits April 10, 2019 13:33

perf(services): avoid rerenders for service status table column

fbd4cc9

perf: avoid service table re-renders if data unchanged

91b464d

natmegs force-pushed the danielmschmidt/DCOS-51377 branch from 21fcdbf to 91b464d Compare April 10, 2019 20:36

DanielMSchmidt closed this Apr 11, 2019

pierrebeitz suggested changes Apr 11, 2019

View reviewed changes

pierrebeitz deleted the danielmschmidt/DCOS-51377 branch August 30, 2019 06:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DCOS-51596: Services Table: avoid rerenders #3792

DCOS-51596: Services Table: avoid rerenders #3792

DanielMSchmidt commented Apr 8, 2019 •

edited by mperrotti

Loading

natmegs commented Apr 8, 2019

natmegs commented Apr 9, 2019

brandonc left a comment

brandonc commented Apr 9, 2019

brandonc commented Apr 9, 2019 •

edited

Loading

DanielMSchmidt commented Apr 10, 2019

DanielMSchmidt commented Apr 10, 2019

GeorgiSTodorov commented Apr 11, 2019

pierrebeitz left a comment

pierrebeitz Apr 11, 2019

pierrebeitz Apr 11, 2019

pierrebeitz Apr 11, 2019

pierrebeitz Apr 11, 2019

pierrebeitz Apr 11, 2019

pierrebeitz Apr 11, 2019

pierrebeitz Apr 11, 2019

pierrebeitz Apr 11, 2019 •

edited

Loading

pierrebeitz Apr 11, 2019

pierrebeitz Apr 11, 2019

DCOS-51596: Services Table: avoid rerenders #3792

DCOS-51596: Services Table: avoid rerenders #3792

Conversation

DanielMSchmidt commented Apr 8, 2019 • edited by mperrotti Loading

Testing

Trade-offs

Dependencies

Screenshots

natmegs commented Apr 8, 2019

natmegs commented Apr 9, 2019

brandonc left a comment

Choose a reason for hiding this comment

brandonc commented Apr 9, 2019

brandonc commented Apr 9, 2019 • edited Loading

DanielMSchmidt commented Apr 10, 2019

DanielMSchmidt commented Apr 10, 2019

GeorgiSTodorov commented Apr 11, 2019

pierrebeitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrebeitz Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielMSchmidt commented Apr 8, 2019 •

edited by mperrotti

Loading

brandonc commented Apr 9, 2019 •

edited

Loading

pierrebeitz Apr 11, 2019 •

edited

Loading