moonchen commented on code in PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#discussion_r3770551673
##########
doc/admin-guide/logging/formatting.en.rst:
##########
@@ -191,6 +193,17 @@ cccs Proxy Cache Cache collapsed connection success;
-1: collapsing was attempted but failed, request went
upstream
0: collapsing was unnecessary
1: attempted to collapse and got a cache hit on
subsequent read attempts
+
+cfl Proxy Cache Freshness limit, in seconds, for an object served from or
+ written to cache. The limit reflects parsed cache response
+ metadata and the effective cache configuration. The value
Review Comment:
"the effective cache configuration" is broader than what
`calculate_document_freshness_limit()` currently includes.
The calculation uses the response headers plus `guaranteed_min/max_lifetime`
and `heuristic_min/max_lifetime`, but not `s->cache_control`. That excludes
`cache.config` directives such as `ttl-in-cache` and `revalidate-after`.
For example, with `ttl-in-cache=300` and an origin response containing
`max-age=1`, a hit at age 3 logs `cfl=1 cca=3`.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3059,6 +3062,7 @@ HttpTransact::build_response_from_cache(State *s,
HTTPWarningCode warning_code)
obj = s->cache_info.object_read;
}
cached_response = obj->response_get();
+ set_cache_freshness_info(s, cached_response, obj->request_sent_time_get(),
obj->response_received_time_get(), true);
Review Comment:
These should use `s->request_sent_time` and `s->response_received_time`
rather than the object's raw timestamps.
`what_is_document_freshness()` uses the transaction timestamps, which are
capped against `s->client_request_time`. The `Age` header sent to the client
uses those values as well. Using the object's raw timestamps here can therefore
make `cca` differ from both the freshness decision and the response's `Age`
header.
There is also a problem on the `api_update_cached_object == CONTINUE` path.
There, `obj` is `&s->cache_info.object_store`, whose timestamps can still be
zero after a plugin creates it with `TSHttpTxnCachedRespModifiableGet()`.
Passing those values to `calculate_document_age()` makes `cca` roughly the
current Unix timestamp.
Passing `s->request_sent_time` and `s->response_received_time` fixes both
cases.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -4835,6 +4839,7 @@
HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// unset warning revalidation failed header if it set
// (potentially added by negative revalidating)
delete_warning_value(base_response, HTTPWarningCode::REVALIDATION_FAILED);
+ set_cache_freshness_info(s, base_response, s->request_sent_time,
s->response_received_time, true);
Review Comment:
This call is skipped by the negative revalidation path earlier in the
function. That branch sets `action = SERVE_AND_UPDATE` and returns before
reaching here.
Those transactions serve the stale cached object and extend its stored
`Expires`, but leave both freshness fields at -1. Negative revalidation is
enabled by default for 500, 502, 503, and 504 responses. There is a reproducer
in the review summary.
--
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]
