Skip to content

Handle lineage graph cycles on the client#2506

Merged
phixMe merged 18 commits into
MarquezProject:mainfrom
jlukenoff:client/lineage/handle-dependency-cycles
Jun 9, 2023
Merged

Handle lineage graph cycles on the client#2506
phixMe merged 18 commits into
MarquezProject:mainfrom
jlukenoff:client/lineage/handle-dependency-cycles

Conversation

@jlukenoff

@jlukenoff jlukenoff commented Jun 8, 2023

Copy link
Copy Markdown
Contributor

Problem

Currently we blow the stack if there is a cycle in the lineage graph.

Closes: #2410

Solution

Memoize the nodes that have been visited so far and only call the recursive function if the node has not yet been visited.

One-line summary:

Checklist

  • You've signed-off your work
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • You've updated any relevant documentation (if relevant)
  • You've included a one-line summary of your change for the CHANGELOG.md (Depending on the change, this may not be necessary).
  • You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)
  • You've included a header in any source code files (if relevant)

@boring-cyborg boring-cyborg Bot added the web label Jun 8, 2023
getSelectedPaths = () => {
const paths = [] as Array<[string, string]>

// Sets used to detect cycles and break out of the recursive loop

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TBD: How would this work if we had a really large cycle with many nodes/paths involved? Perhaps we still run the risk of blowing the stack with this as well.

@codecov

codecov Bot commented Jun 8, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2506 (e859de7) into main (63e11c4) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #2506   +/-   ##
=========================================
  Coverage     83.80%   83.80%           
  Complexity     1233     1233           
=========================================
  Files           235      235           
  Lines          5625     5625           
  Branches        270      270           
=========================================
  Hits           4714     4714           
  Misses          767      767           
  Partials        144      144           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@boring-cyborg boring-cyborg Bot added the docker label Jun 8, 2023
@jlukenoff

Copy link
Copy Markdown
Contributor Author

Confirmed the graph can handle cycles now but will still test a few more scenarios
image

@jlukenoff jlukenoff force-pushed the client/lineage/handle-dependency-cycles branch from 36e0f75 to 2898c28 Compare June 8, 2023 20:47
@jlukenoff

Copy link
Copy Markdown
Contributor Author

Confirmed smaller cycles work as well and don't light up the whole graph erroneously
image

@jlukenoff jlukenoff marked this pull request as ready for review June 8, 2023 20:49
Comment thread docker/metadata.json Outdated
Comment thread web/src/components/lineage/Lineage.tsx
jlukenoff added 7 commits June 8, 2023 14:41
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
@jlukenoff jlukenoff force-pushed the client/lineage/handle-dependency-cycles branch from 89d811a to cf129c3 Compare June 8, 2023 21:41
jlukenoff added 2 commits June 8, 2023 15:31
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Comment thread web/src/__tests__/components/Lineage.test.tsx
jlukenoff added 9 commits June 9, 2023 09:22
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
@jlukenoff jlukenoff requested a review from phixMe June 9, 2023 17:09
@jlukenoff

Copy link
Copy Markdown
Contributor Author

Updated the tests and validated that they fail if I remove my recursive base case. @phixMe lmk if you have any other tests you think we should add to this.

@phixMe phixMe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @jlukenoff Great work!

@phixMe phixMe merged commit 77db9f3 into MarquezProject:main Jun 9, 2023
Xavier-Cliquennois pushed a commit to Xavier-Cliquennois/marquez that referenced this pull request Jul 26, 2023
* init

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* repro

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* remove log lines

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* formatting

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* cleanup

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Create a smaller cycle

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* remove stub metadata

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Test draft

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* try stubbing some methods

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Test rendering component

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* test without mounting

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Formatting

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Remove unnecessary types

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Revert new module installations

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* oops

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Add a test for success

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* dont use ternary

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* improve path checking

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

---------

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Signed-off-by: Xavier-Cliquennois <xavier.cliquennois@wearegraphite.io>
jonathanpmoraes referenced this pull request in nubank/NuMarquez Feb 6, 2025
* init

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* repro

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* remove log lines

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* formatting

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* cleanup

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Create a smaller cycle

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* remove stub metadata

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Test draft

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* try stubbing some methods

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Test rendering component

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* test without mounting

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Formatting

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Remove unnecessary types

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Revert new module installations

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* oops

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* Add a test for success

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* dont use ternary

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

* improve path checking

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

---------

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UI error: max call stack size exceeded

2 participants