Skip to content

feat(pipelines): support workload deployment orders in pipelines#3497

Merged
mergify[bot] merged 10 commits intoaws:mainlinefrom
efekarakus:feat/pipeline-deployments
Apr 27, 2022
Merged

feat(pipelines): support workload deployment orders in pipelines#3497
mergify[bot] merged 10 commits intoaws:mainlinefrom
efekarakus:feat/pipeline-deployments

Conversation

@efekarakus
Copy link
Copy Markdown
Contributor

Resolves #3402

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@efekarakus efekarakus requested a review from a team as a code owner April 22, 2022 19:46
@efekarakus efekarakus requested review from iamhopaul123 and removed request for a team April 22, 2022 19:46
@efekarakus efekarakus force-pushed the feat/pipeline-deployments branch from 8e3f581 to f5f2772 Compare April 22, 2022 20:40
Comment thread internal/pkg/graph/graph.go Outdated
Comment thread internal/pkg/graph/graph.go Outdated
Comment thread internal/pkg/graph/graph.go Outdated
Comment thread internal/pkg/graph/graph.go Outdated
Comment thread internal/pkg/graph/graph_test.go Outdated
Comment thread internal/pkg/graph/graph.go Outdated
Comment thread internal/pkg/graph/graph.go
Comment thread internal/pkg/graph/graph.go Outdated
continue
}
bfs.marked[neighbor] = true
if rank := bfs.ranks[vtx] + 1; rank > bfs.ranks[neighbor] { // Is the new rank higher than a previous traversal?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I might be stupid but I am a little confused here 🥲 Only when bfs.marked[neighbor] is false can the program proceed here, but then does that mean bfs.ranks[neighbor] will be empty too?

I feel like if a vertex is visited, we shouldn't skip too quickly but possibly think about if we should update the rank and update with the lowest rank rather than the highest. For example:

a -> b -> c
d -> c -> f

where both a and d has 0 rank. If we give c rank 2 rather than 1 then f will share the same rank as c.

Copy link
Copy Markdown
Contributor Author

@efekarakus efekarakus Apr 25, 2022

Choose a reason for hiding this comment

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

lol it's not stupid -

but then does that mean bfs.ranks[neighbor] will be empty too?

I think your example shows how it's possible that ranks is not empty. In your example, after a -> b -> c we will have:

map[string]int {
    "a": 0,
    "b": 1,
    "c": 2
}

Then when we start from the second root, d -> c -> f we do not want the rank of c to become 1. It has a higher rank from a previous deployment, so we want to keep it at 2. The final ranks from the algorithm will be:

map[string]int {
   "a": 0,
   "d": 0,
   "b": 1,
   "c": 2,
   "f": 3,
}

We can never set c to have rank 1 otherwise it will run in parallel with b which should happen prior to c. Hope this makes sense!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah now I agree we should set c to 2. However, if we start from d then since c is visited, it won't get updated to a higher rank 2

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.

it will boss because of this if statement.
Since each time traverse is called marked is reset. After starting from d we will have:

ranks: {"d": 0, "c": 1, f: "2"}

Then we reset all marks so c is no longer marked.
When we start from a, in this if-statement we get:

if rank /* 2 */ := bfs.ranks[vtx] /* rank of "b" = 1 */ + 1; rank > bfs.ranks[neighbor ] /*  2 > 1 so update ranks["c"] */

Copy link
Copy Markdown
Contributor

@dannyrandall dannyrandall left a comment

Choose a reason for hiding this comment

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

😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋😋

@efekarakus efekarakus added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. WIP Pull requests that are being modified now. labels Apr 26, 2022
@efekarakus
Copy link
Copy Markdown
Contributor Author

efekarakus commented Apr 26, 2022

boss has a good point where the wrong rank will be calculated for the following scenario:

a -> f  -> d
a -> b -> c -> d

in this scneario, d will have rank 2 instead of 3. So added the label until this test case and a modification is submitted

@efekarakus efekarakus removed WIP Pull requests that are being modified now. do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Apr 26, 2022
@iamhopaul123 iamhopaul123 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 26, 2022
Copy link
Copy Markdown
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

LGTM just a nit 🚢

Comment thread internal/pkg/graph/graph.go Outdated
type Graph[V comparable] struct {
vertices map[V]neighbors[V]
vertices map[V]neighbors[V] // Adjacency list for each vertex.
indegrees map[V]int // Number of incoming edges for each vertex.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
indegrees map[V]int // Number of incoming edges for each vertex.
inDegrees map[V]int // Number of incoming edges for each vertex.

Copy link
Copy Markdown
Contributor

@dannyrandall dannyrandall left a comment

Choose a reason for hiding this comment

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

lgtm!

Comment on lines +50 to +55
arr := make([]V, len(neighbors))
i := 0
for neighbor := range neighbors {
arr[i] = neighbor
i += 1
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
arr := make([]V, len(neighbors))
i := 0
for neighbor := range neighbors {
arr[i] = neighbor
i += 1
}
var arr []V
for neighbor := range neighbors {
arr = append(arr, neighbor)
}

@efekarakus efekarakus removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 26, 2022
@mergify mergify Bot merged commit bdea65b into aws:mainline Apr 27, 2022
@efekarakus efekarakus deleted the feat/pipeline-deployments branch April 27, 2022 01:11
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.

[Design] Ordering deployments in a pipeline

4 participants