Skip to content
This repository has been archived by the owner on May 25, 2022. It is now read-only.

Make pipeline.Operators return an ordered list #480

Closed

Conversation

dehaansa
Copy link

@dehaansa dehaansa commented May 7, 2022

issue with flaky test in opentelemetry-collector-contrib (open-telemetry/opentelemetry-collector-contrib#9761) is due to the inconsistent sorting of operators being returned from pipeline.Operators.

@dehaansa dehaansa requested a review from a team May 7, 2022 02:03
@codecov
Copy link

codecov bot commented May 7, 2022

Codecov Report

Merging #480 (93c96ae) into main (c912ec5) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #480     +/-   ##
=======================================
- Coverage   76.9%   76.8%   -0.2%     
=======================================
  Files         85      85             
  Lines       4153    4156      +3     
=======================================
- Hits        3197    3195      -2     
- Misses       672     676      +4     
- Partials     284     285      +1     
Impacted Files Coverage Δ
pipeline/directed.go 91.0% <100.0%> (-2.9%) ⬇️
operator/input/tcp/tcp.go 78.4% <0.0%> (-1.6%) ⬇️

operators = append(operators, nodes.Node().(OperatorNode).Operator())
// If possible, return the list sorted. If for some strange reason
// an Unorderable error is returned, use the list without guaranteed order
if nodes, err := topo.Sort(p.Graph); err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Nit, but realistically this always works because topo.Sort is tested during instantiation of the pipeline. So I think the comment should specifically state that this is unexpected behavior and be located adjacent to the unreachable case. Roughly:

	if nodes, err := topo.Sort(p.Graph); err == nil {
		for _, node := range nodes {
			operators = append(operators, node.(OperatorNode).Operator())
		}
		return operators
	}

	// Comment
	...

@djaglowski
Copy link
Member

@dehaansa, thanks for opening this. I'm closing it due to the deprecation of the repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants