zwoop commented on code in PR #12782: URL: https://github.com/apache/trafficserver/pull/12782#discussion_r3010332345
########## configs/records.yaml.default.in: ########## @@ -86,6 +86,12 @@ records: # https://docs.trafficserver.apache.org/en/latest/admin-guide/files/records.yaml.en.html#proxy-config-http-cache-when-to-revalidate when_to_revalidate: 0 +# Defer cache open write until response headers are received. Review Comment: Instead of this long comment, shouldn't it just link to the relevant docs anchor? ########## src/proxy/http/HttpTransact.cc: ########## @@ -3333,6 +3333,62 @@ HttpTransact::handle_cache_write_lock_go_to_origin_continue(State *s) } } +/////////////////////////////////////////////////////////////////////////////// +// Name : handle_deferred_cache_write_complete +// Description: Called after cache write opens for deferred cache write. +// We already have the server response headers, so we continue +// with cache write handling. +// +// Possible Next States From Here: +// - handle_cache_operation_on_forward_server_response (if cache write succeeded) +// - handle_no_cache_operation_on_forward_server_response (if cache write failed) +// +/////////////////////////////////////////////////////////////////////////////// +void +HttpTransact::handle_deferred_cache_write_complete(State *s) Review Comment: This one is hard to review :) Are there any existing tests that hits this code (with the setting enabled, obviously)? If not, this feels like we ought to have an autest (ideally start a new proxy-verify "framework" test?). ########## include/proxy/http/HttpConfig.h: ########## @@ -623,6 +623,11 @@ struct OverridableHttpConfigParams { MgmtByte cache_open_write_fail_action = 0; + //////////////////////////////////////////////// + // Defer cache open write until response hdrs // Review Comment: Probably a useful comment, but this is almost too short :) Shouldn't it be something like "...until response headers are received" ? ########## src/proxy/http/HttpTransact.cc: ########## @@ -3378,6 +3434,13 @@ HttpTransact::HandleCacheOpenReadMiss(State *s) s->cache_info.action = CacheAction_t::NO_ACTION; } else if (s->api_server_response_no_store) { // plugin may have decided not to cache the response s->cache_info.action = CacheAction_t::NO_ACTION; + } else if (s->txn_conf->cache_defer_write_on_miss) { + // Defer cache open write until after response headers are received. + // This avoids cache overhead for non-cacheable responses but may Review Comment: In general, I find Claud's comments to be overly verbose. If needed, this feels like it can be a one-line. It's pretty clear that ``` } else if (s->txn_conf->cache_defer_write_on_miss) { ``` will "defer cache open write" ... If the code is self documenting, we shouldn't state the obvious. I'd suggest you look through all the comments (or ask to) and remove the comments that are obvious. ########## src/records/RecordsConfig.cc: ########## @@ -623,6 +623,13 @@ static constexpr RecordElement RecordsConfig[] = // # 6 - retry cache read on write lock failure, if retries exhausted serve stale if allowed, otherwise goto origin {RECT_CONFIG, "proxy.config.http.cache.open_write_fail_action", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , + // # defer_write_on_miss: Review Comment: I don't know that we need this verbose documentation here either, either leave it out, or refer to the Docs section as before. ########## src/proxy/http/HttpTransact.cc: ########## @@ -3333,6 +3333,62 @@ HttpTransact::handle_cache_write_lock_go_to_origin_continue(State *s) } } +/////////////////////////////////////////////////////////////////////////////// +// Name : handle_deferred_cache_write_complete +// Description: Called after cache write opens for deferred cache write. +// We already have the server response headers, so we continue +// with cache write handling. +// +// Possible Next States From Here: +// - handle_cache_operation_on_forward_server_response (if cache write succeeded) +// - handle_no_cache_operation_on_forward_server_response (if cache write failed) +// +/////////////////////////////////////////////////////////////////////////////// +void +HttpTransact::handle_deferred_cache_write_complete(State *s) +{ + TxnDbg(dbg_ctl_http_trans, "handle_deferred_cache_write_complete"); + TxnDbg(dbg_ctl_http_seq, "Entering handle_deferred_cache_write_complete"); + + ink_assert(s->cache_info.action == CacheAction_t::PREPARE_TO_WRITE); + + switch (s->cache_info.write_lock_state) { + case CacheWriteLock_t::SUCCESS: + // Cache write lock acquired successfully + TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock acquired"); + SET_UNPREPARE_CACHE_ACTION(s->cache_info); + // Now we have WRITE action, handle the response with caching + handle_cache_operation_on_forward_server_response(s); + return; + + case CacheWriteLock_t::FAIL: Review Comment: From Claude, sounds like there's a bit of overlap between these two settings, that should at least be documented. - **[`HttpTransact.cc:3364`](https://github.com/apache/trafficserver/blob/21d2cd14f1f4b1dec5e04b3010cd96b544e79988/src/proxy/http/HttpTransact.cc#L3364)**: `handle_deferred_cache_write_complete` FAIL case does not consult `cache_open_write_fail_action`. The normal path in `handle_cache_write_lock` (line 3156) checks this setting and can return 502 via `ERROR_ON_MISS`, `ERROR_ON_MISS_STALE_ON_REVALIDATE`, or `ERROR_ON_MISS_OR_REVALIDATE`. The deferred path unconditionally treats FAIL as `LOCK_MISS` and serves the origin response. This is arguably correct (we already fetched from origin, so returning 502 would be wasteful), but the interaction between `defer_write_on_miss` and `cache_open_write_fail_action` needs to be documented as a known trade-off. Users relying on `ERROR_ON_MISS` for thundering-herd protection should understand that the deferred path bypasses it. ########## src/proxy/http/HttpTransact.cc: ########## @@ -4849,6 +4912,24 @@ HttpTransact::handle_no_cache_operation_on_forward_server_response(State *s) TxnDbg(dbg_ctl_http_trans, "(hncoofsr)"); TxnDbg(dbg_ctl_http_seq, "Entering handle_no_cache_operation_on_forward_server_response"); + // Check if we deferred the cache open write and the response is cacheable. + // If so, we need to open the cache for write now. + if (s->cache_open_write_deferred && s->txn_conf->cache_http) { + bool cacheable = is_response_cacheable(s, &s->hdr_info.client_request, &s->hdr_info.server_response); + TxnDbg(dbg_ctl_http_trans, "[hncoofsr] deferred cache write, response %s cacheable", cacheable ? "is" : "is not"); + + if (cacheable) { + // Response is cacheable - open the cache for write now Review Comment: Again, stating the obvious in this comment, if (cacheable) is self explanatory. ########## include/proxy/http/HttpTransact.h: ########## @@ -697,6 +697,10 @@ class HttpTransact bool is_method_stats_incremented = false; bool skip_ip_allow_yaml = false; + /// True if we deferred opening the cache for write until after response headers. + /// This is an optimization to avoid cache overhead for non-cacheable responses. Review Comment: The second comment here sounds like something that would be documented as part of the new setting. We know why we added this ... ########## src/proxy/http/HttpTransact.cc: ########## @@ -3333,6 +3333,62 @@ HttpTransact::handle_cache_write_lock_go_to_origin_continue(State *s) } } +/////////////////////////////////////////////////////////////////////////////// Review Comment: This is a good comment :) ########## src/proxy/http/HttpTransact.cc: ########## @@ -3333,6 +3333,62 @@ HttpTransact::handle_cache_write_lock_go_to_origin_continue(State *s) } } +/////////////////////////////////////////////////////////////////////////////// +// Name : handle_deferred_cache_write_complete +// Description: Called after cache write opens for deferred cache write. +// We already have the server response headers, so we continue +// with cache write handling. +// +// Possible Next States From Here: +// - handle_cache_operation_on_forward_server_response (if cache write succeeded) +// - handle_no_cache_operation_on_forward_server_response (if cache write failed) +// +/////////////////////////////////////////////////////////////////////////////// +void +HttpTransact::handle_deferred_cache_write_complete(State *s) +{ + TxnDbg(dbg_ctl_http_trans, "handle_deferred_cache_write_complete"); + TxnDbg(dbg_ctl_http_seq, "Entering handle_deferred_cache_write_complete"); + + ink_assert(s->cache_info.action == CacheAction_t::PREPARE_TO_WRITE); + + switch (s->cache_info.write_lock_state) { + case CacheWriteLock_t::SUCCESS: + // Cache write lock acquired successfully + TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock acquired"); + SET_UNPREPARE_CACHE_ACTION(s->cache_info); + // Now we have WRITE action, handle the response with caching + handle_cache_operation_on_forward_server_response(s); + return; + + case CacheWriteLock_t::FAIL: + // Could not get cache write lock, continue without caching + TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock failed, continuing without cache"); + Metrics::Counter::increment(http_rsb.cache_open_write_fail_count); + s->cache_info.action = CacheAction_t::NO_ACTION; + s->cache_info.write_status = CacheWriteStatus_t::LOCK_MISS; + break; + + case CacheWriteLock_t::READ_RETRY: + // This shouldn't happen for deferred write since we don't have an object to read + TxnDbg(dbg_ctl_http_trans, "[hdcwc] unexpected READ_RETRY for deferred write"); + s->cache_info.action = CacheAction_t::NO_ACTION; + s->cache_info.write_status = CacheWriteStatus_t::LOCK_MISS; + break; + + case CacheWriteLock_t::INIT: + default: + ink_release_assert(0); + break; + } + + // If we get here, cache write failed - continue without caching + // The original handle_no_cache_operation_on_forward_server_response will be called Review Comment: Well, yeah, I can see that handle_no_cache_operation_on_forward_server_response is called ... -- 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]
