Copilot commented on code in PR #13611: URL: https://github.com/apache/trafficserver/pull/13611#discussion_r3958186520
########## tests/gold_tests/cache/compat-cache-key.test.py: ########## @@ -0,0 +1,23 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Verify proxy.config.http.cache.try_compat_key_read: objects stored under the +ATS 9.2 cache key are found, revalidated without conditional headers, migrated +to the current key, and then dropped from the legacy key. +''' Review Comment: The test summary says the object is “dropped from the legacy key”, but the PR description and replay (Test 5) explicitly assert the legacy copy is left in place to age out (it just stops being read once the canonical key resolves). Please update this Summary text to match the intended behavior so future readers don’t misinterpret the migration semantics. ########## src/proxy/http/HttpSM.cc: ########## @@ -5279,6 +5283,18 @@ HttpSM::do_range_setup_if_necessary() } } +// YTS Team, yamsat Plugin +// Changed the lookup_url to c_url which enables even +// the new redirect url to perform a CACHE_LOOKUP Review Comment: These newly added comments look like leftover vendor/plugin attribution and don’t accurately describe what the extracted `cache_lookup_url()` helper does (it selects the lookup URL based on redirect state). Consider replacing them with a concise, repo-standard comment describing the function’s behavior, or removing them if they’re not adding useful context. ########## include/proxy/http/HttpSM.h: ########## @@ -344,6 +341,21 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN> void set_http_schedule(Continuation *); int get_http_schedule(int event, void *data); + static CacheHTTPInfo * + cache_write_info_for_lookup(CompatibilityCacheLookup lookup, CacheHTTPInfo *object_read_info) + { + if (lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92) { + return nullptr; + } + return object_read_info; + } Review Comment: This helper hard-codes the `COMPAT_CACHE_LOOKUP_92` enum check while the same header introduces `should_use_compatibility_cache_key()`. Using the shared predicate here would reduce the risk of these checks diverging if compatibility modes expand beyond a single enum value. -- 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]
