Skip to content

Conversation

@candiduslynx
Copy link
Contributor

@candiduslynx candiduslynx commented Jul 4, 2023

Follow-up for #1056

@candiduslynx candiduslynx requested a review from hermanschaaf July 4, 2023 10:13
@candiduslynx candiduslynx requested a review from yevgenypats as a code owner July 4, 2023 10:13
@github-actions github-actions bot added fix and removed fix labels Jul 4, 2023
@hermanschaaf
Copy link
Member

What is this fixing?

@hermanschaaf
Copy link
Member

It seems to me like the other code used to handle a 0 timeout, but this doesn't (I'm not sure what ticker's behavior is if it's passed 0). We don't have tests for that case in these two batch writers right now, but I think a better fix here would be to add a test for a 0 timeout?

@disq
Copy link
Member

disq commented Jul 4, 2023

Not supporting a zero/disabled batchtimeout doesn't sit well with the file destinations (with NoRotate or path patterns without UUID).

@candiduslynx
Copy link
Contributor Author

@hermanschaaf @disq
https://pkg.go.dev/time#Tick
https://cs.opensource.google/go/go/+/refs/tags/go1.20.5:src/time/tick.go;l=68
The impl will ret nil just like the manual implementation that's now removed:

// Tick is a convenience wrapper for NewTicker providing access to the ticking
// channel only. While Tick is useful for clients that have no need to shut down
// the Ticker, be aware that without a way to shut it down the underlying
// Ticker cannot be recovered by the garbage collector; it "leaks".
// Unlike NewTicker, Tick will return nil if d <= 0.
func Tick(d Duration) <-chan Time {
	if d <= 0 {
		return nil
	}
	return NewTicker(d).C
}

Copy link
Member

@disq disq left a comment

Choose a reason for hiding this comment

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

Missing the fix for streamingbatchwriter :)

@hermanschaaf
Copy link
Member

Isn't the leak mentioned in the comment of time.Tick also something consider here?

@hermanschaaf hermanschaaf changed the title fix(ticker): Don't reset manually refactor(ticker): Don't reset manually Jul 4, 2023
@github-actions github-actions bot added refactor and removed fix labels Jul 4, 2023
@disq
Copy link
Member

disq commented Jul 4, 2023

Isn't the leak mentioned in the comment of time.Tick also something consider here?

I think it is if we're going to have long running writers.

@candiduslynx candiduslynx requested a review from disq July 4, 2023 12:58
@candiduslynx
Copy link
Contributor Author

Fixed leak in e63bc5e

@disq
Copy link
Member

disq commented Jul 4, 2023

Same ticker will be used for all writers, maybe move this to the upper writers package? We use MsgID from that as well.

@candiduslynx
Copy link
Contributor Author

@disq

Same ticker will be used for all writers, maybe move this to the upper writers package? We use MsgID from that as well.

Done in 68015d3

Missing the fix for streamingbatchwriter :)

Done in 8f241a0

@kodiakhq kodiakhq bot merged commit 646d451 into main Jul 4, 2023
@kodiakhq kodiakhq bot deleted the chore/ticker branch July 4, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants