search

mtesseract / nakadi-client

Haskell Client Library for the Nakadi Event Broker
Other
13 stars 9 forks source link

Concurrent consumers #94

Closed mtesseract closed 6 years ago

mtesseract commented 6 years ago

Here it comes. Subscriptions/Events.hs became to big, which is why I have decided to do some refactoring as well.

mtesseract commented 6 years ago

At some point the subscription publishing/consumption parts of the test suite should be made less error-prone when it comes to timing issues in the test logic.

Feel free to give it a review, @etorreborre :-D

etorreborre commented 6 years ago

I need more time to review the main logic after a good night' sleep. I'll try to do that tomorrow.

mtesseract commented 6 years ago

Sure, looking forward to that! Let me know if something is unclear or should be documented better.

etorreborre commented 6 years ago

Sorry, still no time... Maybe tomorrow :-).

etorreborre commented 6 years ago

Finally I got a bit of time to add some comments. I feel that some "encapsulation" would be nice to isolate a bit the committing logic from the structure / maps supporting it. Maybe this is my object-oriented experience speaking here but I think that would help in understanding the code.

mtesseract commented 6 years ago

Thanks for your review, I will look at your comments as soon as possible. By the way, on my work notebook I did some measurements and I was able to consume events at approx. 15700 E/s using 8 partitions / workers. This is, against a dockerized Nakadi running on localhost. I am quite happy about this, as this is not even optimized yet.

mtesseract commented 6 years ago

@etorreborre I have updated the PR. But I think I still need to figure out what exactly you have in mind in terms of encapsulating. Generally speaking, I like encapsulation, so I see value in that. I cannot really comment on the OO background remark, as I don't really have an OO background. :-/

mtesseract commented 6 years ago

I have now isolated the committing strategies.

etorreborre commented 6 years ago

Hi @mtesseract I haven't the time for an in-depth review but I scrolled through the changes and the extraction of the various components looks good to me. I just left a comment about a refactoring opportunity (I think).

mtesseract commented 6 years ago

@etorreborre, I have reduced the code duplication that you have spotted. :-) Would you then be fine with merging or would you feel uncomfortable with it because of lack of time?

mtesseract commented 6 years ago

Any update on this?

etorreborre commented 6 years ago

No new review from me, nor the time to try it on our current setup. I will try to give it a go when I load test our next service consuming events from SPP because this might add a good boost in performance when we will bootstrap the service.

mtesseract commented 6 years ago

I will open a new PR for this.