search

keenlabs / KeenClient-Java

Official Java client for the Keen IO API. Build analytics features directly into your Java apps.
https://keen.io/docs
MIT License
74 stars 43 forks source link

Max Attempts #29

Closed smurthas closed 9 years ago

smurthas commented 9 years ago

This is just a re-submit of #28 so that the diff is more clear.

Geeber commented 9 years ago

Sorry it took me a while to get back to you on this; I think it looks good. It's a little hard for me to follow all the conversation threads, but I believe there were two open questions:

(a) Handling of failures while writing the attempts file. I think the path you took seems fine. (b) Synchronization of get/setAttempts. I think we should probably add locking KeenClient. In general people aren't gonna be adding events in a tight loop so the performance overhead of the locks should be small, especially compared to file access times. That said, in the Java server case where a RAM store is being used the overhead could be more relevant. So it would be nice if we can add the locks in a way that they'll only be called if the eventStore is a KeenAttemptCountingEventStore. That might require slight refactoring, because the getAttempts/setAttempts calls in queueEvent have to be atomic. Should be doable though.

If there's anything else I forgot or that you want me to double-check, let me know! Otherwise I think once locking is in place we can merge.

smurthas commented 9 years ago

I cleaned up the exception handling logic and added some as-needed synchronization. One thing to note -- since RamEventStore, TestEventStore, and FileEventStore are all KeenAttemptCountingEventStore implementations, the tests don't cover the code paths that handle regular event stores. Given that we are very likely going to refactor this abstraction in the coming months, I'm not sure it makes sense to add coverage for those, but I'm happy to if you think it's important.

I think this should be good to go.

Geeber commented 9 years ago

:shipit: (feel free to merge into the 2.1.0 branch)