[GitHub] [incubator-zipkin] Logic-32 commented on issue #2502: Adding storage-throttle module to address "over capacity" issues

[GitBox <gi...@apache.org>]

Logic-32 commented on issue #2502: Adding storage-throttle module to address "over capacity" issues
URL: https://github.com/apache/incubator-zipkin/pull/2502#issuecomment-488136796
 
 
   > There is a fair amount of code with experiential notes in. There aren't enough tests for some of this, especially things like pool resizing we should have tests or notes on how people are going to resize...
   >
   > Functionality wise, this looks ok.. we need to up the coverage a bit and also do a multithreaded case .like pool of 10 consumers to prove throttling works under contension.. this could help smoke out any thread safety bugs.
   
   A majority of the tests currently reside in ThrottledCall.  Testing ThrottledStorageComponent is proving to be quite difficult due to how Netflix's concurrency-limit works.  Reviewing some of their tests, it looks like a certain amount of "latency" is required for the limiter to function as expected.  So I can't simply submit tasks that either pass/fail and expect the limit to go up/down.
   
   If the additional tests in ThrottledCall are not sufficient then I'd have to request we do some brainstorming of some kind.  The only route I see to testing ThrottledStorageComponent more would involve some significant refactoring to inject mocks/etc. which I'm not sure I can dedicate the time to :(

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services