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

Fix Pods number in graph not updating (minor label) #2728

Merged
merged 2 commits into from
Jul 21, 2017
Merged

Fix Pods number in graph not updating (minor label) #2728

merged 2 commits into from
Jul 21, 2017

Conversation

rndstr
Copy link
Contributor

@rndstr rndstr commented Jul 19, 2017

Fixes #2401.

fbarl
fbarl reviewed Jul 19, 2017
View reviewed changes
client/app/scripts/charts/nodes-layout.js Outdated
@@ -402,7 +402,7 @@ function cloneLayout(layout, nodes, edges) {
function copyLayoutProperties(layout, nodeCache, edgeCache) {
const result = Object.assign({}, layout);
result.nodes = layout.nodes.map(node => (nodeCache.has(node.get('id'))
? node.merge(nodeCache.get(node.get('id'))) : node));
? nodeCache.get(node.get('id')).merge(node) : node));

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@rndstr
Fix Pods number in graph not updating (minor label)
e2d5a2a 
By only caching layout values (x, y).

Fixes #2401.
@rndstr rndstr force-pushed the 2401-pods-number-not-updating-on-labelMinor branch from d2c341c to e2d5a2a Compare July 19, 2017 16:00
@fbarl fbarl self-requested a review July 20, 2017 08:55
@fbarl
Copy link
Contributor

fbarl commented Jul 20, 2017 •
edited
Loading

Actually, nodeCache seems to be used in a lot of places in nodes-layout.js, namely in functions:

  • hasUnseenNodes
  • hasNewNodesOfExistingRank
  • doLayoutNewNodesOfExistingRank
  • (copyLayoutProperties)

The problem is that except for the x and y fields, rank field seems to be used in some of them as well for some of the heuristic algorithms we use to bypass full layout recalculation. As far as I can see, rank is the only such field, but it would be good to double check that.

So, to make sure nothing is broken I think we could do one of the following:

  1. Cache rank for nodes alongside [x, y] if it's a fixed property of a node (not sure about that, but maybe @2opremio knows)
  2. See if we could replace nodeCache with immNodes in the functions listed above

I think a quick fix would be to go with 1 but it's important to note that we'd still be assuming that ranks are immutable, which deserves a comment in the code even if it's always true.

Edit: I opened #2733 when we get some extra time to restructure that code on a more ground-level. In the meantime, I think a quick fix in this PR that fixes the bug is ok.

fbarl
fbarl suggested changes Jul 20, 2017
View reviewed changes
Copy link
Contributor

@fbarl fbarl left a comment

Choose a reason for hiding this comment

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

rank field still needs to be considered.

fbarl
fbarl reviewed Jul 20, 2017
View reviewed changes
client/app/scripts/charts/nodes-layout.js Outdated
@@ -487,7 +487,7 @@ export function doLayout(immNodes, immEdges, opts) {

// cache results
cache.cachedLayout = layout;
cache.nodeCache = cache.nodeCache.merge(layout.nodes);
cache.nodeCache = cache.nodeCache.merge(layout.nodes.map(n => fromJS({ x: n.get('x'), y: n.get('y') })));

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@fbarl fbarl mentioned this pull request Jul 20, 2017
Open
fbarl
fbarl reviewed Jul 21, 2017
View reviewed changes
client/app/scripts/charts/nodes-layout.js Outdated
// only cache layout-related properties
// NB: These properties must be immutable wrt a given node because properties of updated nodes
// will be overwritten with the cached values, see copyLayoutProperties()
cache.nodeCache = cache.nodeCache.merge(layout.nodes.map(n => pick(n.toJS(), ['x', 'y', 'rank'])));

This comment was marked as abuse.

Sign in to view

@rndstr rndstr force-pushed the 2401-pods-number-not-updating-on-labelMinor branch from bcd81aa to 96265a4 Compare July 21, 2017 14:00
fbarl
fbarl approved these changes Jul 21, 2017
View reviewed changes
Copy link
Contributor

@fbarl fbarl left a comment

Choose a reason for hiding this comment

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

LGTM!

@rndstr rndstr merged commit 00ddb51 into master Jul 21, 2017
@rndstr rndstr deleted the 2401-pods-number-not-updating-on-labelMinor branch July 21, 2017 14:34
@rndstr rndstr mentioned this pull request Jul 24, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbarl fbarl fbarl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rndstr @fbarl