Add retry to in-memory storage system (close #156)

[Miranda Wilson (mscwilson)]

For issue #156.

This (breaking) change adds a backoff and retry mechanism to the Emitter. Previously, events that failed to send were returned, so that the user could choose to use a callback to track them again. Now, when a request fails, the events are returned to the event buffer for retry in a subsequent request.

When a batch of events (TrackerPayloads) is removed from the event buffer for sending, the events are now copied into a hashmap, and turned into a BatchPayload, a new wrapper class. BatchPayload stores the TrackerPayloads for the request along with the hashmap key. If the request is sent successfully, the batched events are deleted from the hashmap. If not, they are inserted back at the front of the event buffer. The aim is to send events approximately first in, first out.

The backoff mechanism uses the schedule() of ScheduledThreadPoolExecutor to delay all new requests after a failure has occurred. The initial delay is 50 ms, which increases exponentially with every subsequent failure. When one request succeeds, the retry delay returns to 0.

For testing purposes, it made sense here to allow users to set the InMemoryEventStore event buffer capacity on creation. The LinkedBlockingDeque by default has a capacity of Integer.MAX_VALUE.

Having a bufferCapacity field made the name bufferSize to mean batchSize extra confusing, so I changed it. It's not really part of retry though, so I might move these commits to their own issues. This name change is now issue #306.

While testing this, I discovered that processing Events into TrackerPayloads in the Tracker is very fast. This means that the Tracker threadpool I introduced in PR #293 isn't necessary. I've removed it (making PR #298 irrelevant).

Note to community contributor Paul Laturaze (@AcidFlow): we merged in your PR #259 in version 0.11.0 which allowed users to select their own ExecutorService. My current changes now require the ScheduledExecutorService interface. Any thoughts on this change?

[Miranda Wilson (mscwilson)]

This bit needs work. If the queue is full, and we want to drop the newer events, then this method must be able to hold the eventBuffer long enough to removeLast and offerFirst all the failed events. Otherwise more events will arrive and it will get full again.

[AlexBenny]

yeah, we can add a GH issue. This is not a regression as the same problem was present before. In theory we could check the size before to add considering not only the queue size but also the pending events. It seems a bit too much for this PR so I would just move this problem in a separate issue.

[Miranda Wilson (mscwilson)]

Issue #308

[AlexBenny]

Awesome! I've just browsed the code and added some comments.
I'd like to run the code and look into it a bit deeper before to approve, hope you can wait just another day.

[AlexBenny]

yeah, we can add a GH issue. This is not a regression as the same problem was present before. In theory we could check the size before to add considering not only the queue size but also the pending events. It seems a bit too much for this PR so I would just move this problem in a separate issue.

[Paul Laturaze (AcidFlow)]

Hey Miranda Wilson (@mscwilson) !

I haven't look at the whole changelog yet as I don't have much time, but regarding your change now requiring a ScheduledExecutorService I think this should be fine. IIRC my previous contribution was to give more control on the Executor service so you could control its thread factory, pool and rejected execution handler so users would control the amount of thread they use, and could add metrics on rejection for instance. Using a ScheduledExecutorService still gives the user the same control with more reliability thanks to your changes :)

[Miranda Wilson (mscwilson)]

Great, thanks Paul Laturaze (@AcidFlow)!

[AlexBenny]

Great job! 👍 👍 👍

I think it's a great result. Making the track method synchronous has reduced a bit the performance for the tracking side but shouldn't be a big problem. The benchmarking shows longer time for the track method. The track method synchronous helps to avoid too much complexity with two different thread-pools. The unique emitter thread-pool seems simple and fast (we don't have a benchmark here but you did some manual testing). I think the current design is a good compromise between performance and extensibility of the tracker.

I think we should deprecate the SimpleEmitter if we don't remove it.

[AlexBenny]

This seems a weird notation for warnings. I would update it to @implNote (https://nipafx.dev/javadoc-tags-apiNote-implSpec-implNote/)

[Miranda Wilson (mscwilson)]

Unfortunately implNote is a non-standard javadoc tag! It breaks the build. I couldn't see an obvious way to specify it as a tag in the build.gradle, so for now I've added html into the javadoc comments instead.