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

Option to limit file size? #168

Open
danqing opened this issue Jul 24, 2017 · 9 comments
Open

Option to limit file size? #168

danqing opened this issue Jul 24, 2017 · 9 comments

Comments

@danqing
Copy link

danqing commented Jul 24, 2017

Is it possible to provide the functionality to limit the underlying file size (automatically remove oldest element if the file reaches a certain size and new elements are coming)?

If there's no intention to provide such functionality maybe it's possible to change some function to protected (e.g. expandIfNecessary) so a subclass can add it?

Thanks!

@f2prateek
Copy link
Collaborator

Funnily enough, I was just think about this last weekend. At Segment we limit the size of the file at the application layer to 15MB artificially — each queued event must be <15kb and we remove the oldest element if the queue size is over 1000.

I think there are two APIs we could offer for this:

  1. Limit based on size of the file.
  2. Limit based on number of entries (this could be done via a decorator).

@danqing
Copy link
Author

danqing commented Jul 24, 2017

Yeah I can see both being very useful. It'd be great to be able to make sure that the file won't blow up, for one (esp. on mobile. also large files seem to have issues per #150). And when the queue keeps growing it's a sign that the backlog may never be cleared at the current processing speed, so being able to drop elements may be very important.

@JakeWharton
Copy link
Member

JakeWharton commented Jul 24, 2017 via email

@danqing
Copy link
Author

danqing commented Jul 24, 2017

Personally I'd like to see auto-eviction. When a size limit is set and we are about to go over it (i.e. when we absolutely need to discard items), discarding the oldest one seem to make the most sense to me (also since we are in FIFO world).

I'm not sure what @f2prateek would like - if auto-eviction is the most common, maybe the expandIfNecessary method can be made inheritable for custom overrides (if needed), and auto-eviction being provided with the library?

@f2prateek
Copy link
Collaborator

In Segment's case, eviction makes the most sense for us. But I see Jake's point that the desired behaviour might be different for different use cases. For instance with payments, it would likely make more sense to block the insertion and notify the user the current transaction failed, rather than drop a queued transaction.

@danqing QueueFile isn't designed for extension the way you've described. What I'd do is either wrap QueueFile or ObjectQueue in your own decorator for the add methods and apply your logic to evict entries or block insertion based on file size or number of entries.

@danqing
Copy link
Author

danqing commented Jul 24, 2017

So based on my very limited understanding, as long as we can swap logic for expandIfNecessary we will be able to control what happens when the limit is reached?

It seems to be the thing called when we hit the limit, and currently the behavior is to double the size. If it can be swapped, then a class can:

  • drop oldest element instead of doubling size -> eviction
  • return false of some sort to block insertion -> I'm actually not entirely sure what this means. If I'm hitting the limit and new data is still being generated, I need to discard that new data. Otherwise the total data size is still going up. Or are you and Jake suggesting that it can be used to disable some UI, etc. altogether (i.e. stop data creation)?
  • delegate to a callback

Do you suggest creating additional methods to short-circuit expandIfNecessary? I wonder if that will introduce some dupe logic..

@jsachania
Copy link

Do you have any suggested workaround to limit the file size without modifying the current code? I see that you have com.squareup.tape.QueueFile#usedBytes which is private. Using this we could prevent from adding more elements to the queue.

@NightlyNexus
Copy link
Contributor

@jsachania Could you make a delegating ObjectQueue that checks the file size in add? An imperfect workaround but doable with existing APIs.

@jsachania
Copy link

I think checking the file size may not work because Tape doubles the file size when it runs out of room in the file. We need to know the actual bytes used or remaining bytes to make estimate.

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

5 participants