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

Only re-create pod donut if data changes #6899

Merged

Conversation

jeff-phillips-18
Copy link
Member

Fixes
https://issues.redhat.com/browse/ODC-5001

Description
Currently, the pod donut chart is updated whenever the pod resource object changes. Update to determine if the visible data has changed and only update when it does.

Non-functional change

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

/kind cleanup

@openshift-ci-robot openshift-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Oct 12, 2020
@openshift-ci-robot openshift-ci-robot added the component/shared Related to console-shared label Oct 12, 2020
Copy link
Contributor

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

/lgtm Can't say I follow all the refactoring required for this, but code looks solid and ran dev branch locally and verfied Deployments -> Deployment Details -> Pod Ring renders, although not sure how to test if reload only happens on donut data changes. Launched Actions -> Edit Pod Count, changed count, submitted modal, this also triggered Pod Ring chart to re-render.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2020
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2020
@jerolimov
Copy link
Member

Hi @jeff-phillips-18, I played a little bit with a dozen of Pods/Deployments and debugged in PodStatus a little bit.

I noticed that when I changed one Pod (edited the ReplicaSet of one which recreates then the Pod) that all ChartDonuts are recalculated. Which means the factory of const chartDonut = React.useMemo(.. was called here.

This happen because the titleComponent still "changed" => It shows the same data but the reference changed.

I tried to add an additional useMemo around the getTitleComponent function in pod-ring-utils.ts here, like this one:

const getTitleComponent = (
  longTitle: boolean = false,
  longSubtitle: boolean = false,
  reversed: boolean = false,
) => {
  // eslint-disable-next-line react-hooks/rules-of-hooks
  return React.useMemo(() => {
    const labelClasses = classNames('pf-chart-donut-title', {
      'pod-ring__center-text--reversed': reversed,
      'pod-ring__center-text': !reversed,
      'pod-ring__long-text': longTitle,
    });
    return React.createElement(ChartLabel, {
      dy: longSubtitle ? -5 : 0,
      style: { lineHeight: '11px' },
      className: labelClasses,
    });
  }, [longTitle, longSubtitle, reversed]);
};

With this useMemo the titleComponent in your PodStatus was not changed anymore (without a reason) and the chartDonut dependency list works fine and return the memorized component.

Anything else which I tested locally works as expected. So I add a lgtm anyway.

But what do you think about the recommendation above? Should we create a new PR for this? Maybe with some more refactorings, so that it works without disabling the react-hooks eslint rule. Whats you opinion here? :)

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. component/dev-console Related to dev-console and removed lgtm Indicates that a PR is ready to be merged. labels Oct 20, 2020
@jeff-phillips-18
Copy link
Member Author

@jerolimov Added a commit to make a hook for getting the pod ring label. PTAL.

@jerolimov
Copy link
Member

Cool new hooks, thanks for implementing this so spontaneously and fixing our loop issue as well. 👍

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtaylor113, jeff-phillips-18, jerolimov, rohitkrai03

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2020
@openshift-merge-robot openshift-merge-robot merged commit 44c2256 into openshift:master Oct 21, 2020
@spadgett spadgett added this to the v4.7 milestone Oct 26, 2020
@jeff-phillips-18 jeff-phillips-18 deleted the pod-status-update branch December 2, 2020 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/shared Related to console-shared kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants