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

Pending flushes don't flush during periods of inactivity #45

Open
timrobertson100 opened this issue May 1, 2013 · 6 comments
Open

Pending flushes don't flush during periods of inactivity #45

timrobertson100 opened this issue May 1, 2013 · 6 comments

Comments

@timrobertson100
Copy link
Contributor

During the flush cycle batches are handed over to a the flush thread pool.
Batches are flushed when the size threshold is breached, or if the TTL expires. However, the TTL is only checked when a new Op is added.
During periods of inactivity we notice inaccurate counts which is undesirable.
It would be good if the TTL flush occurred even when no new Ops are incoming.

I would propose adding a monitor thread that checked every minute or so if there were any flushes pending. Thoughts?

[we need this for graceful shutdown reasons - currently we loose batches in flight]

@drevell
Copy link
Contributor

drevell commented May 1, 2013

SGTM.

My only two cents: this seems like the kind of thing where thread safety could be tricky. Proper synchronization is necessary when accessing batches in flight. (end of nagging)

@todd534
Copy link
Contributor

todd534 commented May 1, 2013

Seems like a fine idea, but wouldn't a more direct solution to your
specific problem be to flush() as part of the shutdown routine?

@timrobertson100
Copy link
Contributor Author

Hey Todd.
On shutdown yeah (actually I think we do), but still during idle time there is nothing to force a flush until the next Op comes in.
Of course we could push the monitor thread that calls the flush() outside of DC if you suggest that is more elegant?

@todd534
Copy link
Contributor

todd534 commented May 1, 2013

No, I like the monitor thread idea, I just was trying to be helpful in
the time between now and that getting merged :)

@todd534
Copy link
Contributor

todd534 commented May 1, 2013

One thing that I would like to see is if the operation of the monitor was optional.

@drevell don't you think that if the batch in flight is only accessed through flush() that already wraps up the synchronization issues?

@drevell
Copy link
Contributor

drevell commented May 6, 2013

Sure, a monitor thread sounds fine, as long as there's no race between checking if a batch needs to be flushed and deciding to flush it; do we need a conditional flush that holds a lock?

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

No branches or pull requests

3 participants