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 2021/02/04 21:34:38 UTC
[GitHub] [trafficserver] jrushford opened a new pull request #7485: PR to prevent thundering parent retries.
jrushford opened a new pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485
This PR updates parent selection to limit the number of simultaneous
transactions that will retry a parent once it's retry_time window
has elapsed. The limiting prevents a thundering retry of a parent by
hundreds of transactions that could very will hang if the parent is
still unreachable following retry_time expiration, The config parameter
'proxy.config.http.parent_proxy.max_trans_retries' is added to let the
operator decide how many transactions will be allowed to retry a
parent when it's retry time has elapsed. The parameter defaults to 4.
We have had this patch deployed to production canaries for about 2 weeks
now and it is functioning as intended. We are beginning to roll it out into
production soon.
----------------------------------------------------------------
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
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r582952464
##########
File path: proxy/ParentConsistentHash.cc
##########
@@ -389,6 +391,7 @@ ParentConsistentHash::markParentUp(ParentResult *result)
ink_atomic_swap(&pRec->failedAt, static_cast<time_t>(0));
int old_count = ink_atomic_swap(&pRec->failCount, 0);
+ ink_atomic_swap(&pRec->retriers, 0);
Review comment:
For 8 byte (or less) values there is no need for an atomic store because it is intrinsically atomic. However, there is `std::atomic<T>::store` for the general case.
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-778524374
[approve ci cutest]
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r581195975
##########
File path: proxy/ParentConsistentHash.cc
##########
@@ -389,6 +391,7 @@ ParentConsistentHash::markParentUp(ParentResult *result)
ink_atomic_swap(&pRec->failedAt, static_cast<time_t>(0));
int old_count = ink_atomic_swap(&pRec->failCount, 0);
+ ink_atomic_swap(&pRec->retriers, 0);
Review comment:
@SolidWallOfCode were marking the parent up and just need to reset retriers to 0, the old value from ink_atomic_swap is not really needed.
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r582226807
##########
File path: proxy/ParentConsistentHash.cc
##########
@@ -389,6 +391,7 @@ ParentConsistentHash::markParentUp(ParentResult *result)
ink_atomic_swap(&pRec->failedAt, static_cast<time_t>(0));
int old_count = ink_atomic_swap(&pRec->failCount, 0);
+ ink_atomic_swap(&pRec->retriers, 0);
Review comment:
@SolidWallOfCode I removed the ink_atomic's and changed to std::atomic's
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r581195975
##########
File path: proxy/ParentConsistentHash.cc
##########
@@ -389,6 +391,7 @@ ParentConsistentHash::markParentUp(ParentResult *result)
ink_atomic_swap(&pRec->failedAt, static_cast<time_t>(0));
int old_count = ink_atomic_swap(&pRec->failCount, 0);
+ ink_atomic_swap(&pRec->retriers, 0);
Review comment:
@SolidWallOfCode were marking the parent up and just need to reset retriers to 0, the old value from ink_atomic_swap is not really needed and there is no ink_atomic_store()
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r571054406
##########
File path: doc/admin-guide/files/records.config.en.rst
##########
@@ -1194,6 +1194,11 @@ Parent Proxy Configuration
The amount of time allowed between connection retries to a parent cache that is unavailable.
+.. ts:cv:: CONFIG proxy.config.http.parent_proxy.max_trans_retries INT 4
+
+ Limits the number of simultaneous transactions that may retry a parent once the parents
Review comment:
It's for all transactions to that parent on that parent.config line. If a retry is successful, the parent is restored.
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r581193791
##########
File path: proxy/ParentSelection.cc
##########
@@ -1117,6 +1124,8 @@ EXCLUSIVE_REGRESSION_TEST(PARENTSELECTION)(RegressionTest * /* t ATS_UNUSED */,
_st.setHostStatus("curly", HOST_STATUS_UP, 0, Reason::MANUAL);
// Test 1
+ SET_MAX_RETRIERS(20);
+ RecSetRecordInt("proxy.config.http.parent_proxy.max_trans_retries", 20, REC_SOURCE_DEFAULT);
Review comment:
@SolidWallOfCode forgot to delete line 1128, i'll fix it.
----------------------------------------------------------------
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
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r580319906
##########
File path: proxy/ParentConsistentHash.cc
##########
@@ -389,6 +391,7 @@ ParentConsistentHash::markParentUp(ParentResult *result)
ink_atomic_swap(&pRec->failedAt, static_cast<time_t>(0));
int old_count = ink_atomic_swap(&pRec->failCount, 0);
+ ink_atomic_swap(&pRec->retriers, 0);
Review comment:
I don't see a point to using `atomic_swap` if the return value isn't checked.
----------------------------------------------------------------
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
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r580320559
##########
File path: proxy/ParentSelection.cc
##########
@@ -1105,6 +1107,11 @@ EXCLUSIVE_REGRESSION_TEST(PARENTSELECTION)(RegressionTest * /* t ATS_UNUSED */,
params->findParent(request, result, fail_threshold, retry_time); \
} while (0)
+#define SET_MAX_RETRIERS(x) \
Review comment:
This should be an inline function, not a macro.
----------------------------------------------------------------
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
[GitHub] [trafficserver] mtorluemke commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
mtorluemke commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r571050274
##########
File path: doc/admin-guide/files/records.config.en.rst
##########
@@ -1194,6 +1194,11 @@ Parent Proxy Configuration
The amount of time allowed between connection retries to a parent cache that is unavailable.
+.. ts:cv:: CONFIG proxy.config.http.parent_proxy.max_trans_retries INT 4
+
+ Limits the number of simultaneous transactions that may retry a parent once the parents
Review comment:
Is this for a single object (if using consistent_hash), or for all objects to that parent?
----------------------------------------------------------------
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
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-783434076
I would say my biggest concern is the continued use of the old internal atomics instead of `std::atomic`. Beyond that, I have some specific concerns.
----------------------------------------------------------------
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
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r580319142
##########
File path: proxy/ParentConsistentHash.cc
##########
@@ -231,9 +231,11 @@ ParentConsistentHash::selectParent(bool first_call, ParentResult *result, Reques
// check if the host is retryable. It's retryable if the retry window has elapsed
// and the global host status is HOST_STATUS_UP
if (pRec && !pRec->available && host_stat == HOST_STATUS_UP) {
- Debug("parent_select", "Parent.failedAt = %u, retry = %u, xact_start = %u", (unsigned int)pRec->failedAt,
- (unsigned int)retry_time, (unsigned int)request_info->xact_start);
- if ((pRec->failedAt + retry_time) < request_info->xact_start) {
+ Debug("parent_select", "Parent.failedAt = %u, retry = %u, retriers = %d, max_retriers = %u, xact_start = %u",
+ static_cast<unsigned>(pRec->failedAt), static_cast<unsigned>(retry_time), pRec->retriers, max_retriers,
+ static_cast<unsigned>(request_info->xact_start));
+ if (((pRec->failedAt + retry_time) < request_info->xact_start) && (pRec->retriers < max_retriers) &&
+ ink_atomic_increment(&pRec->retriers, 1) < max_retriers) {
Review comment:
There's no point in doing the pre-check because there can still be an increment on another thread between that and the actual atomic increment. This merely narrows the window. To be correct, you would do better with something like
```
if (ink_atomic_increment(&pRec->retriers, 1) < max_retriers) {
// ...
} else {
ink_atomic_increment(&pRec->retriers, -1);
}
```
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r581199876
##########
File path: proxy/ParentConsistentHash.cc
##########
@@ -231,9 +231,11 @@ ParentConsistentHash::selectParent(bool first_call, ParentResult *result, Reques
// check if the host is retryable. It's retryable if the retry window has elapsed
// and the global host status is HOST_STATUS_UP
if (pRec && !pRec->available && host_stat == HOST_STATUS_UP) {
- Debug("parent_select", "Parent.failedAt = %u, retry = %u, xact_start = %u", (unsigned int)pRec->failedAt,
- (unsigned int)retry_time, (unsigned int)request_info->xact_start);
- if ((pRec->failedAt + retry_time) < request_info->xact_start) {
+ Debug("parent_select", "Parent.failedAt = %u, retry = %u, retriers = %d, max_retriers = %u, xact_start = %u",
+ static_cast<unsigned>(pRec->failedAt), static_cast<unsigned>(retry_time), pRec->retriers, max_retriers,
+ static_cast<unsigned>(request_info->xact_start));
+ if (((pRec->failedAt + retry_time) < request_info->xact_start) && (pRec->retriers < max_retriers) &&
+ ink_atomic_increment(&pRec->retriers, 1) < max_retriers) {
Review comment:
@SolidWallOfCode thanks, I'll test this and make the change.
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford merged pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford merged pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485
----------------------------------------------------------------
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
[GitHub] [trafficserver] bneradt commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-779965885
[approve ci autest]
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford edited a comment on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford edited a comment on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-778524374
[approve ci autest]
----------------------------------------------------------------
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
[GitHub] [trafficserver] bryancall commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-777842088
@jrushford Says it has been in production for 3 weeks already.
----------------------------------------------------------------
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
[GitHub] [trafficserver] bryancall commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-777841949
@SolidWallOfCode is going to take a look at it.
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-778524889
[approve ci autest]
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r571054406
##########
File path: doc/admin-guide/files/records.config.en.rst
##########
@@ -1194,6 +1194,11 @@ Parent Proxy Configuration
The amount of time allowed between connection retries to a parent cache that is unavailable.
+.. ts:cv:: CONFIG proxy.config.http.parent_proxy.max_trans_retries INT 4
+
+ Limits the number of simultaneous transactions that may retry a parent once the parents
Review comment:
It's for all transactions to that parent on that parent.config line.
----------------------------------------------------------------
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
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r580321036
##########
File path: proxy/ParentSelection.cc
##########
@@ -1117,6 +1124,8 @@ EXCLUSIVE_REGRESSION_TEST(PARENTSELECTION)(RegressionTest * /* t ATS_UNUSED */,
_st.setHostStatus("curly", HOST_STATUS_UP, 0, Reason::MANUAL);
// Test 1
+ SET_MAX_RETRIERS(20);
+ RecSetRecordInt("proxy.config.http.parent_proxy.max_trans_retries", 20, REC_SOURCE_DEFAULT);
Review comment:
Why is this done twice?
----------------------------------------------------------------
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
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-786012103
Minor comments - `var.fetch_add(fetch_add(1, std::memory_order_relaxed)` can be replaced with `var++`. It's one of the nicer features of atomics that you can get pre or post increment values atomically via pre or post fix increment. `fetch_add` becomes useful when the value isn't exactly 1.
----------------------------------------------------------------
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
[GitHub] [trafficserver] bneradt commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-780012729
[approve ci autest]
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on a change in pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#discussion_r581201879
##########
File path: proxy/ParentSelection.cc
##########
@@ -1105,6 +1107,11 @@ EXCLUSIVE_REGRESSION_TEST(PARENTSELECTION)(RegressionTest * /* t ATS_UNUSED */,
params->findParent(request, result, fail_threshold, retry_time); \
} while (0)
+#define SET_MAX_RETRIERS(x) \
Review comment:
@SolidWallOfCode For now, I was using the macro to be consistent with the rest of the regression tests, there are 215 regression tests all using half a dozen macro's. I think it would be best if I create an issue and change the parent selection regression tests to use the Catch test framework instead.
----------------------------------------------------------------
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
[GitHub] [trafficserver] jrushford commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-785165921
> I would say my biggest concern is the continued use of the old internal atomics instead of `std::atomic`. Beyond that, I have some specific concerns.
@SolidWallOfCode Ahh, I see. I'll switch to using std::atomic in the ParentSelection code.
----------------------------------------------------------------
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
[GitHub] [trafficserver] zwoop commented on pull request #7485: PR to prevent thundering parent retries.
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7485:
URL: https://github.com/apache/trafficserver/pull/7485#issuecomment-832904673
Cherry-picked to v9.1.x branch.
--
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