You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/01/07 21:18:58 UTC

[GitHub] [trafficserver] traeak opened a new pull request #8591: add proxy.config.http.cache.max_open_write_retry_timeout parameter

traeak opened a new pull request #8591:
URL: https://github.com/apache/trafficserver/pull/8591


   While performing synthetic load testing with an origin that still accepts connections but stalls, we encountered larger than expected transaction ttms.
   
   Using the following setting I was getting too much variation in time spent spinning on the write lock.  Anywhere from 1.5s to 8s.  
   {code}
   CONFIG proxy.config.http.cache.max_open_write_retries INT 150
   {code}
   
   So I added this millisecond timeout:
   {code}
   CONFIG proxy.config.http.cache.max_open_write_retry_timeout INT 1500
   {code}
   
   When set this timeout is used in preference to the open_write_retries.


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

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] ezelkow1 commented on pull request #8591: add proxy.config.http.cache.max_open_write_retry_timeout parameter

Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #8591:
URL: https://github.com/apache/trafficserver/pull/8591#issuecomment-1045222325


   [approve ci rocky]


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

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] bryancall commented on pull request #8591: add proxy.config.http.cache.max_open_write_retry_timeout parameter

Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8591:
URL: https://github.com/apache/trafficserver/pull/8591#issuecomment-1081249918


   @SolidWallOfCode is going to take a look at this


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

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] bryancall commented on pull request #8591: add proxy.config.http.cache.max_open_write_retry_timeout parameter

Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8591:
URL: https://github.com/apache/trafficserver/pull/8591#issuecomment-1081250114


   @cmcfarlen is going to take a look at this


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

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] cmcfarlen commented on a change in pull request #8591: add proxy.config.http.cache.max_open_write_retry_timeout parameter

Posted by GitBox <gi...@apache.org>.
cmcfarlen commented on a change in pull request #8591:
URL: https://github.com/apache/trafficserver/pull/8591#discussion_r839953545



##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -139,18 +139,20 @@ HttpCacheSM::state_cache_open_read(int event, void *data)
     }
     break;
 
-  case EVENT_INTERVAL:
+  case EVENT_INTERVAL: {
     // Retry the cache open read if the number retries is less
     // than or equal to the max number of open read retries,
     // else treat as a cache miss.
-    ink_assert(open_read_tries <= master_sm->t_state.txn_conf->max_cache_open_read_retries || write_locked);
+    bool const retries_done = (master_sm->t_state.txn_conf->max_cache_open_read_retries < open_read_tries);
+    ink_assert(retries_done || write_locked);

Review comment:
       Maybe the read retry count is checked and handled somewhere else.  In that case, disregard the second part of the comment above.

##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -139,18 +139,20 @@ HttpCacheSM::state_cache_open_read(int event, void *data)
     }
     break;
 
-  case EVENT_INTERVAL:
+  case EVENT_INTERVAL: {
     // Retry the cache open read if the number retries is less
     // than or equal to the max number of open read retries,
     // else treat as a cache miss.
-    ink_assert(open_read_tries <= master_sm->t_state.txn_conf->max_cache_open_read_retries || write_locked);
+    bool const retries_done = (master_sm->t_state.txn_conf->max_cache_open_read_retries < open_read_tries);
+    ink_assert(retries_done || write_locked);

Review comment:
       This logic looks wrong to me.  If the max is 4 and open_read_tries is 1, then retries_done will be false (4 is not less than 1).  If write_locked is false, then this will be `false || false` which will trigger the assert.  Perhaps this should be `ink_assert(!retries_done || write_locked)`?
   
   Further, I see this assert, but I don't see that the call to `do_cache_open_read` is ever skipped.  In debug mode, going past the max will abort the process and in release mode it will always retry.  Perhaps this should go into an error state after retries_done is true?
   




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

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficserver] bryancall edited a comment on pull request #8591: add proxy.config.http.cache.max_open_write_retry_timeout parameter

Posted by GitBox <gi...@apache.org>.
bryancall edited a comment on pull request #8591:
URL: https://github.com/apache/trafficserver/pull/8591#issuecomment-1081249918


   @SolidWallOfCode is going to try to take a look at this


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

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org