Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix non-unique ID used in worker pool #6212

Merged
merged 3 commits into from
Jan 22, 2024
Merged

Fix non-unique ID used in worker pool #6212

merged 3 commits into from
Jan 22, 2024

Conversation

thampiotr
Copy link
Contributor

@thampiotr thampiotr commented Jan 22, 2024 •
edited

PR Description

  • Add a test in the first commit
  • Add a fix in the second commit

Which issue(s) this PR fixes

Fixes #6199

Notes to the Reviewer

Wrote a slightly different, smaller test than the draft branch mentioned on the issue.

Before the fix, running go test ./pkg/flow/ -count 10 -test.run "TestUpdates_TwoModules*" would usually catch the bug:

--- FAIL: TestUpdates_TwoModules_SameCompNames (3.01s)
    module_caching_test.go:200:
        	Error Trace:	/Users/piotr/workspace/agent/pkg/flow/module_caching_test.go:200
        	Error:      	Condition never satisfied
        	Test:       	TestUpdates_TwoModules_SameCompNames
FAIL
FAIL	github.com/grafana/agent/pkg/flow	19.395s
FAIL

After the fix, it doesn't fail.

PR Checklist

  • CHANGELOG.md updated
  • Tests updated

@thampiotr thampiotr marked this pull request as ready for review January 22, 2024 12:59
tpaschalis
tpaschalis approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@tpaschalis tpaschalis left a comment

Choose a reason for hiding this comment

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

LGTM.

wildum
wildum approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@wildum wildum left a comment

Choose a reason for hiding this comment

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

LGTM, nice test. Changelog entry could be a bit more precise but that's not a blocker

CHANGELOG.md Outdated Show resolved Hide resolved
@thampiotr thampiotr force-pushed the thampiotr/fix-non-uniqu-id-used branch from 982d321 to 9081b70 Compare January 22, 2024 13:32
wildum
wildum reviewed Jan 22, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@thampiotr thampiotr force-pushed the thampiotr/fix-non-uniqu-id-used branch from 9081b70 to a4eecb5 Compare January 22, 2024 13:41
@thampiotr thampiotr merged commit efb8144 into main Jan 22, 2024
9 of 10 checks passed
@thampiotr thampiotr deleted the thampiotr/fix-non-uniqu-id-used branch January 22, 2024 13:57
BarunKGP pushed a commit to BarunKGP/grafana-agent that referenced this pull request Feb 20, 2024
@thampiotr @BarunKGP
Fix non-unique ID used in worker pool (grafana#6212)
c25236f 
* Test to reproduce the bug

* Fix the bug

* changelog
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wildum wildum wildum approved these changes

@tpaschalis tpaschalis tpaschalis approved these changes

@rfratto rfratto Awaiting requested review from rfratto

Assignees

@wildum wildum

Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nested modules sometimes fail to propagate exports
3 participants
@thampiotr @tpaschalis @wildum