Open danqing opened 7 years ago
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:
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.
What behavior would you expect here though when you're at the queue size? Auto-evict oldest element? Block the insertion? Give a callback with the expunged elements? Something else?
On Mon, Jul 24, 2017 at 8:47 PM Danqing Liu notifications@github.com wrote:
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 https://github.com/square/tape/issues/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.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/square/tape/issues/168#issuecomment-317533983, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEdjZ-UP_6SEMZvot93nx_etuYqWQks5sRPTbgaJpZM4Ohnwh .
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?
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.
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:
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)?Do you suggest creating additional methods to short-circuit expandIfNecessary
? I wonder if that will introduce some dupe logic..
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.
@jsachania Could you make a delegating ObjectQueue that checks the file size in add
? An imperfect workaround but doable with existing APIs.
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.
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!