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

Add graph complexity check on page load #1994

Merged
merged 1 commit into from
Nov 10, 2016
Merged

Conversation

jpellizzari
Copy link
Contributor

Fix for #1721 .

When the page is loaded, the first received topology will trigger a check to see if the graph will be too complex to render. If so, the view will switch to the table view. The complexity check formula:
(currentTopology.stats.node_count * (2 * currentTopology.stats.edge_count)) > 500

The user can switch to the graph view at any time. This logic only applies at page load. To test with a high node count (on linux):
nc -lk 1122 & for i in $(seq 1 300) ; do nc localhost 1122 & done

@davkal Let me know what you think.

@@ -34,7 +34,8 @@ describe('NodeDetails', () => {
details = {label: 'Node 1'};
const c = TestUtils.renderIntoDocument(
<Provider store={configureStore()}>
<NodeDetails nodes={nodes}
<NodeDetails topologyId="containers" nodes={nodes}

This comment was marked as abuse.

This comment was marked as abuse.

@@ -33,6 +34,7 @@ export const initialState = makeMap({
gridSortedDesc: null,
highlightedEdgeIds: makeSet(),
highlightedNodeIds: makeSet(),
initialPageLoad: true,

This comment was marked as abuse.

@@ -1,2 +1,3 @@

export const EDGE_ID_SEPARATOR = '-';
export const GRAPH_COMPLEXITY_THRESH = 500;

This comment was marked as abuse.

This comment was marked as abuse.

@davkal
Copy link
Contributor

davkal commented Nov 9, 2016

Code looks good.

One more thing: if Scope has been used before, the view mode may be stored in localStorage (see router-utils.js and storage-utils.js). Should we then take those values, or still check for the graph complexity?

@jpellizzari
Copy link
Contributor Author

@davkal Since the graph could have gotten more complex from one browser session to the next, I think we should switch to the table mode no matter what the URL or localStorage says.

@jpellizzari jpellizzari force-pushed the 1721-high-node-count branch 4 times, most recently from fcdc97f to 87d5ae8 Compare November 9, 2016 18:38
@davkal
Copy link
Contributor

davkal commented Nov 10, 2016

LGTM

@davkal davkal assigned jpellizzari and unassigned davkal Nov 10, 2016
@jpellizzari jpellizzari merged commit 0d58a23 into master Nov 10, 2016
@jpellizzari jpellizzari deleted the 1721-high-node-count branch November 10, 2016 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants