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

diskqueue: support a callback when the disk queue is empty #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jehiah
Copy link
Member

@jehiah jehiah commented Nov 25, 2020

This supports nsqio/nsq#1305

@jehiah jehiah self-assigned this Nov 25, 2020
Copy link
Member

@ploxiln ploxiln left a comment

Choose a reason for hiding this comment

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

No real objections, looks good to me.

}

// New instantiates an instance of diskQueue, retrieving metadata
// from the filesystem and starting the read ahead goroutine
func New(name string, dataPath string, maxBytesPerFile int64,
minMsgSize int32, maxMsgSize int32,
syncEvery int64, syncTimeout time.Duration, logf AppLogFunc) Interface {
syncEvery int64, syncTimeout time.Duration, logf AppLogFunc, cb EmptyCallback) Interface {
Copy link
Member

Choose a reason for hiding this comment

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

I have quibbles with the naming here ... the type EmptyCallback func() initially sounds to me like it is describing the callback taking no arguments and returning nothing. I lean towards just queueEmptyCB func() (no extra type).

It's a bit annoying that we have to break compatibility when adding optional arguments here. Already did that for logf though, and not sure want to redesign with a DiskQueueOptions struct or similar, right now.

@jehiah
Copy link
Member Author

jehiah commented Dec 1, 2020

I thought i would need this feedback loop - but i'm not currently using it in nsqio/nsq#1305 (still a WIP so somewhat subject to change) - that means I might just end up closing this.

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.

2 participants