ywkaras commented on code in PR #9620:
URL: https://github.com/apache/trafficserver/pull/9620#discussion_r1175998678
##########
proxy/http/HttpTransact.h:
##########
@@ -944,7 +944,22 @@ class HttpTransact
MgmtInt
configured_connect_attempts_max_retries() const
{
- return txn_conf->connect_attempts_max_retries;
+ if (dns_info.looking_up != ResolveInfo::PARENT_PROXY) {
+ return txn_conf->connect_attempts_max_retries;
+ }
+ // For parent proxy, return the maximum attempt count for the current
+ // parent intead of the global max retries for the whole parent group.
+ // The max attempt count for the current parent is calculated by rounding
+ // the current attempt up to next multiple of ppca.
+ auto ppca = txn_conf->per_parent_connect_attempts;
+ auto cur_attempts = current.attempts.get();
+ auto cur_parent_max_attempts = ((cur_attempts + ppca - 1) / ppca) * ppca;
+ if (cur_attempts == cur_parent_max_attempts) {
+ // If the current attempt is already a multiple of ppca, get the next
+ // multiple.
+ cur_parent_max_attempts += ppca;
+ }
Review Comment:
This call:
https://github.com/apache/trafficserver/blob/6e8933f35ebf5a50211e3b06aa52749ac0ec3c3b/proxy/http/HttpTransact.cc#L3646
can't cause an assert. `configured_connect_attempts_max_retries()` can't be
more than `txn_conf->parent_connect_attempts`. The test in the `if` statement
at line 3644 ensures that an assert is impossible if execution reaches line
3644.
The only other call to the increment function is
https://github.com/apache/trafficserver/blob/6e8933f35ebf5a50211e3b06aa52749ac0ec3c3b/proxy/http/HttpTransact.cc#L3865.
Not clear under what circumstances this call is reached. The very large
HttpTransact and HttpSM classes have little internal scoping or even
encapsulation. It could takes days or weeks to analyze possible code paths.
As I previously said, it's very hard to work on a source bases like this
without taking leaps of faith and making reasonable assumptions. So I suggest
making the reasonable assumption that a retry will be blocked if the number of
retries is already at the maximum. To back that up, you could write long Au
test(s) exercising parent.config (as I did for strategies.yaml). But if
parent.config is soon to be deprecated, it's questionable if the effort
involved is worth 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
