Skip to content

Commit 3da84f9

Browse files
deryrahmanarinda-arif
deryrahman
authored and
arinda-arif
committed
feat: improve logs on cyclic deps (#562)
* feat: improve logs on cyclic deps * refactor: stringify cyclic paths * refactor: use treeprint * fix: lint * refactor: combine cyclic validation into one function * refactor: rename to nodeName
1 parent aa16c89 commit 3da84f9

File tree

5 files changed

+101
-48
lines changed

5 files changed

+101
-48
lines changed

‎core/tree/multi_root_tree.go‎

Lines changed: 71 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@ package tree
33
import (
44
"errors"
55
"fmt"
6+
"sort"
7+
8+
"github.com/xlab/treeprint"
69
)
710

811
// ErrCyclicDependencyEncountered is triggered a tree has a cyclic dependency
@@ -46,51 +49,86 @@ func (t *MultiRootTree) GetNodeByName(dagName string) (*TreeNode, bool) {
4649
return value, ok
4750
}
4851

49-
// IsCyclic - detects if there are any cycles in the tree
50-
func (t *MultiRootTree) IsCyclic() error {
51-
visitedMap := make(map[string]bool)
52-
for _, node := range t.dataMap {
53-
if _, visited := visitedMap[node.GetName()]; !visited {
54-
pathMap := make(map[string]bool)
55-
err := t.hasCycle(node, visitedMap, pathMap)
56-
if err != nil {
57-
return err
58-
}
59-
}
52+
// get sorted nodeNames from dataMap
53+
func (t *MultiRootTree) getSortedNodeNames() []string {
54+
nodeNames := []string{}
55+
for nodeName := range t.dataMap {
56+
nodeNames = append(nodeNames, nodeName)
6057
}
61-
return nil
58+
sort.Strings(nodeNames)
59+
return nodeNames
6260
}
6361

64-
// runs a DFS on a given tree using visitor pattern
65-
func (t *MultiRootTree) hasCycle(root *TreeNode, visited, pathMap map[string]bool) error {
66-
_, isNodeVisited := visited[root.GetName()]
67-
if !isNodeVisited || !visited[root.GetName()] {
68-
pathMap[root.GetName()] = true
69-
visited[root.GetName()] = true
70-
var cyclicErr error
71-
for _, child := range root.Dependents {
72-
n, _ := t.GetNodeByName(child.GetName())
73-
_, isChildVisited := visited[child.GetName()]
74-
if !isChildVisited || !visited[child.GetName()] {
75-
cyclicErr = t.hasCycle(n, visited, pathMap)
62+
// ValidateCyclic - detects if there are any cycles in the tree
63+
func (t *MultiRootTree) ValidateCyclic() error {
64+
// runs a DFS on a given tree using visitor pattern
65+
var checkCyclic func(*TreeNode, map[string]bool, map[string]bool, *[]string) error
66+
checkCyclic = func(node *TreeNode, visitedNodeNames, visitedPaths map[string]bool, orderedVisitedPaths *[]string) error {
67+
_, isNodeVisited := visitedNodeNames[node.GetName()]
68+
if !isNodeVisited || !visitedNodeNames[node.GetName()] {
69+
visitedPaths[node.GetName()] = true
70+
visitedNodeNames[node.GetName()] = true
71+
*orderedVisitedPaths = append(*orderedVisitedPaths, node.GetName())
72+
var cyclicErr error
73+
for _, child := range node.Dependents {
74+
n, _ := t.GetNodeByName(child.GetName())
75+
_, isChildVisited := visitedNodeNames[child.GetName()]
76+
if !isChildVisited || !visitedNodeNames[child.GetName()] {
77+
cyclicErr = checkCyclic(n, visitedNodeNames, visitedPaths, orderedVisitedPaths)
78+
}
79+
if cyclicErr != nil {
80+
return cyclicErr
81+
}
82+
83+
if isVisited, ok := visitedPaths[child.GetName()]; ok && isVisited {
84+
*orderedVisitedPaths = append(*orderedVisitedPaths, child.GetName())
85+
cyclicErr = fmt.Errorf("%w: %s", ErrCyclicDependencyEncountered, prettifyPaths(*orderedVisitedPaths))
86+
}
87+
if cyclicErr != nil {
88+
return cyclicErr
89+
}
7690
}
77-
if cyclicErr != nil {
78-
return cyclicErr
91+
visitedPaths[node.GetName()] = false
92+
i := 0
93+
for i < len(*orderedVisitedPaths) && (*orderedVisitedPaths)[i] != node.GetName() {
94+
i++
7995
}
96+
*orderedVisitedPaths = append((*orderedVisitedPaths)[:i], (*orderedVisitedPaths)[i+1:]...)
97+
}
98+
return nil
99+
}
80100

81-
_, childAlreadyInPath := pathMap[child.GetName()] // 1 -> 2 -> 1
82-
if childAlreadyInPath && pathMap[child.GetName()] {
83-
cyclicErr = fmt.Errorf("%w: %s", ErrCyclicDependencyEncountered, root.GetName())
84-
}
85-
if cyclicErr != nil {
86-
return cyclicErr
101+
visitedNodeNames := make(map[string]bool)
102+
nodeNames := t.getSortedNodeNames()
103+
for _, nodeName := range nodeNames {
104+
node := t.dataMap[nodeName]
105+
if _, visited := visitedNodeNames[node.GetName()]; !visited {
106+
visitedPaths := map[string]bool{}
107+
orderedVisitedPaths := []string{}
108+
if err := checkCyclic(node, visitedNodeNames, visitedPaths, &orderedVisitedPaths); err != nil {
109+
return err
87110
}
88111
}
89-
pathMap[root.GetName()] = false
90112
}
113+
91114
return nil
92115
}
93116

117+
func prettifyPaths(paths []string) string {
118+
if len(paths) == 0 {
119+
return ""
120+
}
121+
i := len(paths) - 1
122+
root := treeprint.NewWithRoot(paths[i])
123+
tree := root
124+
125+
for i--; i >= 0; i-- {
126+
tree = tree.AddBranch(paths[i])
127+
}
128+
129+
return "\n" + root.String()
130+
}
131+
94132
// NewMultiRootTree returns an instance of multi root dag tree
95133
func NewMultiRootTree() *MultiRootTree {
96134
return &MultiRootTree{

‎core/tree/multi_root_tree_test.go‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ func TestMultiRootDagTree(t *testing.T) {
2323
multiRootTree.AddNodeIfNotExist(treeNode1)
2424
multiRootTree.AddNodeIfNotExist(treeNode2)
2525

26-
err := multiRootTree.IsCyclic()
26+
err := multiRootTree.ValidateCyclic()
2727
assert.NotNil(t, err)
2828
assert.Contains(t, err.Error(), tree.ErrCyclicDependencyEncountered.Error())
2929
})
@@ -38,22 +38,37 @@ func TestMultiRootDagTree(t *testing.T) {
3838
assert.Equal(t, 1, len(rootNodes))
3939
assert.Equal(t, "job1", rootNodes[0].Data.GetName())
4040
})
41-
t.Run("IsCyclic", func(t *testing.T) {
41+
t.Run("ValidateCyclic", func(t *testing.T) {
4242
t.Run("should throw an error if cyclic", func(t *testing.T) {
4343
treeNode1 := tree.NewTreeNode(models.JobSpec{
44-
Name: "job1",
44+
Name: "pilotdata-integration.playground.job1",
4545
})
4646
treeNode2 := tree.NewTreeNode(models.JobSpec{
47-
Name: "job2",
47+
Name: "pilotdata-integration.playground.job2",
48+
})
49+
treeNode3 := tree.NewTreeNode(models.JobSpec{
50+
Name: "pilotdata-integration.playground.job3",
51+
})
52+
treeNode4 := tree.NewTreeNode(models.JobSpec{
53+
Name: "pilotdata-integration.playground.job4",
4854
})
4955
multiRootTree := tree.NewMultiRootTree()
5056
multiRootTree.AddNode(treeNode1)
5157
multiRootTree.AddNode(treeNode2)
52-
treeNode1.AddDependent(treeNode2)
58+
multiRootTree.AddNode(treeNode3)
59+
multiRootTree.AddNode(treeNode4)
60+
treeNode4.AddDependent(treeNode3)
61+
treeNode3.AddDependent(treeNode2)
5362
treeNode2.AddDependent(treeNode1)
54-
err := multiRootTree.IsCyclic()
63+
treeNode2.AddDependent(treeNode4)
64+
err := multiRootTree.ValidateCyclic()
5565
assert.NotNil(t, err)
56-
assert.Contains(t, err.Error(), "cycle dependency")
66+
assert.Equal(t, `a cycle dependency encountered in the tree:
67+
pilotdata-integration.playground.job2
68+
└── pilotdata-integration.playground.job3
69+
└── pilotdata-integration.playground.job4
70+
└── pilotdata-integration.playground.job2
71+
`, err.Error())
5772
})
5873
t.Run("should not return error if not cyclic", func(t *testing.T) {
5974
treeNode1 := tree.NewTreeNode(models.JobSpec{
@@ -66,7 +81,7 @@ func TestMultiRootDagTree(t *testing.T) {
6681
multiRootTree.AddNode(treeNode1)
6782
multiRootTree.AddNode(treeNode2)
6883
treeNode1.AddDependent(treeNode2)
69-
err := multiRootTree.IsCyclic()
84+
err := multiRootTree.ValidateCyclic()
7085
assert.Nil(t, err)
7186
})
7287
})

‎job/priority_resolver.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ func (a *priorityResolver) buildMultiRootDependencyTree(jobSpecs []models.JobSpe
147147
}
148148
}
149149

150-
if err := multiRootTree.IsCyclic(); err != nil {
150+
if err := multiRootTree.ValidateCyclic(); err != nil {
151151
return nil, err
152152
}
153153
return multiRootTree, nil

‎job/priority_resolver_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,7 @@ func TestMultiRootDAGTree(t *testing.T) {
529529
dagTree.AddNode(node2)
530530
dagTree.AddNode(node3)
531531

532-
err := dagTree.IsCyclic()
532+
err := dagTree.ValidateCyclic()
533533
assert.NotNil(t, err)
534534
assert.Contains(t, err.Error(), tree.ErrCyclicDependencyEncountered.Error())
535535
})
@@ -589,7 +589,7 @@ func TestMultiRootDAGTree(t *testing.T) {
589589
dagTree.AddNode(node1211)
590590
dagTree.AddNode(node1212)
591591

592-
err := dagTree.IsCyclic()
592+
err := dagTree.ValidateCyclic()
593593
assert.Nil(t, err)
594594

595595
depsMap := map[*tree.TreeNode]int{
@@ -614,7 +614,7 @@ func TestMultiRootDAGTree(t *testing.T) {
614614
dagTree.AddNode(node2)
615615
dagTree.MarkRoot(node2)
616616

617-
err := dagTree.IsCyclic()
617+
err := dagTree.ValidateCyclic()
618618
assert.Nil(t, err)
619619
})
620620

@@ -625,7 +625,7 @@ func TestMultiRootDAGTree(t *testing.T) {
625625
dagTree := tree.NewMultiRootTree()
626626
dagTree.AddNode(node2)
627627

628-
err := dagTree.IsCyclic()
628+
err := dagTree.ValidateCyclic()
629629
assert.Nil(t, err)
630630
})
631631

@@ -658,7 +658,7 @@ func TestMultiRootDAGTree(t *testing.T) {
658658
dagTree.AddNode(node31)
659659
dagTree.AddNode(node41)
660660

661-
err := dagTree.IsCyclic()
661+
err := dagTree.ValidateCyclic()
662662
assert.NotNil(t, err)
663663
assert.Contains(t, err.Error(), tree.ErrCyclicDependencyEncountered.Error())
664664
})

‎job/service.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -669,7 +669,7 @@ func populateDownstreamDAGs(dagTree *tree.MultiRootTree, jobSpec models.JobSpec,
669669
}
670670
}
671671

672-
if err := dagTree.IsCyclic(); err != nil {
672+
if err := dagTree.ValidateCyclic(); err != nil {
673673
return nil, err
674674
}
675675

0 commit comments

Comments
 (0)