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.

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

fix(discovery tests): allow requireUpdate util to timeout in discovery/file… #13380

Merged
merged 1 commit into from Jan 18, 2024

Conversation

machine424
Copy link
Collaborator

…/file_test.go.

The loop ran indefinitely if the condition isn't met.

Before, each iteration created a new timer channel which was always outpaced by the other timer channel with smaller duration.

minor detail: There was a memory leak: resources of the ~10 previous timers were constantly kept. With the fix, we may keep the resources of one timer around for defaultWait but this isn't worth the changes to make it right.

…/file_test.go.

The loop ran indefinitely if the condition isn't met.

Before, each iteration created a new timer channel which was always outpaced by
the other timer channel with smaller duration.

minor detail: There was a memory leak: resources of the ~10 previous timers were
constantly kept. With the fix, we may keep the resources of one timer around for defaultWait
but this isn't worth the changes to make it right.

Signed-off-by: machine424 <ayoubmrini424@gmail.com>
@machine424
Copy link
Collaborator Author

To reproduce the issue:

@@ -383,7 +382,7 @@ func TestFileUpdate(t *testing.T) {
        defer runner.stop()
 
        // Verify that we receive the initial target groups.
-       runner.requireUpdate(time.Time{}, validTg(sdFile))
+       runner.requireUpdate(time.Time{}, []*targetgroup.Group{})
 
        // Verify that we receive an update with the new target groups.

And run the test.
(I didn't think this is worth an issue, but tell me if I should create one)

@machine424
Copy link
Collaborator Author

cc @bboreham
Another easy one.

@bboreham bboreham changed the title fix(discovery): allow requireUpdate util to timeout in discovery/file… fix(discovery tests): allow requireUpdate util to timeout in discovery/file… Jan 18, 2024
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Good catch, thanks.

@bboreham bboreham merged commit b9952ff into prometheus:main Jan 18, 2024
24 checks passed
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.

None yet

2 participants