[GitHub] [solr] atris commented on pull request #96: SOLR-15056: add circuit breaker for CPU, fix load circuit breaker

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

atris commented on pull request #96:
URL: https://github.com/apache/solr/pull/96#issuecomment-852387820


   > The existing CPU circuit breaker has an incorrect name, so yes, it is moved to a more accurate name.
   > 
   > Load average can include things besides CPU. In some systems, it includes threads in iowait. So it should not have “CPU” in the name.
   > 
   > The documentation explains what happens when the CPU circuit breaker is not available.
   > 
   > Yes, the tests are similar because the two circuit breakers take similar inputs.
   
   
   Thank you for clarifying.
   
   My stand still remains the same -- it is a good practice to explicitly list what metrics the CPU circuit breaker is using.
   
   For the load average, I agree that it can include auxiliary inputs but always contains CPU as a factor, hence the CPU in name suggestion. It is not a blocker though.
   
   In general, I have two objections:
   
   1. API changes for the new circuit breaker -- this is causing unnecessary noise across a lot of files.
   
   2. Duplication of code -- a significant amount of code is duplicated between load average circuit breaker and the new circuit breaker.
   
   Please address these issues and iterate.


-- 
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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org