chore(graph): add traverse methods container start and shutdown #5508
Conversation
KollaAdithya
requested review from
Lou1415926
and removed request for
a team
|
🍕 Here are the new binary sizes!
|
KollaAdithya
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## mainline #5508 +/- ##
============================================
+ Coverage 70.00% 70.05% +0.04%
============================================
Files 302 302
Lines 46169 46325 +156
Branches 309 309
============================================
+ Hits 32321 32453 +132
- Misses 12276 12294 +18
- Partials 1572 1578 +6 ☔ View full report in Codecov by Sentry. |
internal/pkg/graph/graph.go
Outdated
| @@ -17,6 +24,8 @@ const ( | |||
| type Graph[V comparable] struct { | |||
| vertices map[V]neighbors[V] // Adjacency list for each vertex. | |||
| inDegrees map[V]int // Number of incoming edges for each vertex. | |||
| status map[V]string // status of each vertex. | |||
There was a problem hiding this comment.
It's a little odd for a generic graph to have a "status" field. Maybe consider creating a more specific struct and inherit from Graph[V comparable] in this pkg? This way we can also consolidate graphTraversal which is already very deeply coupled with this struct.
There was a problem hiding this comment.
I just created a new LabeledGraph[V comparable] which inherits *Graph[V] that would reduce the coupling between Graph[V comparable].
Does this address the concern on coupling and generic graph to have a "status" field 🤔
internal/pkg/graph/graph.go
Outdated
| @@ -17,6 +24,8 @@ const ( | |||
| type Graph[V comparable] struct { | |||
| vertices map[V]neighbors[V] // Adjacency list for each vertex. | |||
| inDegrees map[V]int // Number of incoming edges for each vertex. | |||
| status map[V]string // status of each vertex. | |||
| lock sync.Mutex // lock used to mutate graph data. | |||
There was a problem hiding this comment.
Why do we need a lock in the graph struct instead of defining it in the caller? It doesn't seem we apply the lock to all methods to make this struct truly atomic...
There was a problem hiding this comment.
I modified the code where every method of LabeledGraph[V comparable] struct uses lock .
Couple of reasons why i want to use lock declaring over here instead of caller.
- If the lock is managed within the data structure, it simplifies the usage for the caller. The caller does not need to worry about acquiring and releasing the lock, making the API easier to use
- The caller may not have full context or understanding of the graph's internals, which increases the risk of incorrect or insufficient locking, leading to potential data races or deadlocks.
iamhopaul123
added
the
do-not-merge
KollaAdithya
force-pushed
the
implement/traversal
branch
from
b994e9d to
36dde93
Compare
KollaAdithya
removed
the
do-not-merge
This Is part3 for implementation of essential and dependsOn
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.