phongn commented on PR #13257: URL: https://github.com/apache/trafficserver/pull/13257#issuecomment-5703572623
Thanks — and thank you for rebuilding it rather than reading it. All six are addressed in `ebb773b8`. Taking them in order of how wrong I was. ## 1. The zstd shim — you are right, and it was my regression I deleted it on a Linux-only check and generalised. CMake's config-mode search looks for `ZSTDConfig.cmake` or `zstd-config.cmake`; zstd installs `zstdConfig.cmake`. On a case-sensitive filesystem that matches neither, which is what I tested, and I concluded "dead everywhere" — missing that `ZSTDConfig.cmake` and `zstdConfig.cmake` are the *same file* on a case-insensitive one. Your Homebrew repro is the case I reasoned away. I reproduced it here rather than taking it on faith, by simulating the config package on Linux and toggling only the filename: | config filename present | `ZSTD_FOUND` | `zstd::zstd` | |---|---|---| | `zstdConfig.cmake` only (case-sensitive) | FALSE | — | | also matching `ZSTDConfig.cmake` (what macOS gives you) | 1 | **MISSING** | So `HAVE_ZSTD_H` goes true and the four link sites fail at generate time, exactly as you described. Restored as a loop over `zstd::libzstd_shared` / `_static` / `libzstd`. One change from the original: if the config package exports none of those names, it now warns and clears `HAVE_ZSTD_H` instead of leaving a dangling `zstd::zstd`, so the failure mode is "built without zstd" rather than a generate error. Verified both paths against the simulated package: `zstd::zstd RESOLVES` with `HAVE_ZSTD_H=1` under `PREFER_CONFIG`, and the module path unchanged. Description fixed too, and expanded — the sentence you flagged was true again once the shim came back, but it did not say *why* the shim exists, which is how it got deleted. It now spells out the case-insensitive filename match. ## 2. Decompress failure paths — test added, and it earned its place You were right that nothing reached them. `CLFUS reports a corrupted compressed entry rather than serving it` stores a payload, runs `compress_entries()`, overwrites the stored blob, then asserts a miss, `ram_cache_decompress_failures` incremented by exactly one, and the entry gone from the cache. Parametrized over all five codecs. Reaching the entry needs a `friend struct RamCacheCLFUSTestAccess` on `RamCacheCLFUS`, declared with a comment saying nothing in the product uses it. It found more than coverage: it exercises the per-codec detail text for real, and all five produce a distinct, useful message. ``` uncompress: data error lzma_stream_buffer_decode returned 7, wrote 0 of 262144 output bytes LZ4_decompress_safe returned -1050, expected 262144 ZSTD_decompressDCtx: Unknown frame descriptor ``` I re-ran your mutation. `if (false)` on the lz4 `l != rc` check now fails the test. Your zstd mutation still passes, and there is a reason worth recording: that path has two checks, and disabling `ZSTD_isError(ll)` alone leaves `l != ll` to catch it — the corrupt frame is still never served, it just reports "produced 18446744073709551606 bytes, expected 262144" instead of the error name. Disabling both does fail the test. So zstd is covered; your single-check mutation survived because of the second check, not a gap. ## 3. The 256 KB compare — worse than green-without-`-s` `./RamCacheCLFUS -s` was not merely unhelpful, it was a hard failure: `test_RamCacheCLFUS.cc:238: FAILED: due to unexpected exception with messages: basic_string::_M_create`, with a screenful of raw payload bytes before it. `-s` passes cleanly now. Replaced with a size check plus a `first_difference()` helper, so a real round-trip break reports the offset as a number — `65 == 262144` rather than an exception. ## 4. lz4 floor — raised to 1.7.5 Raised. I could not verify the requirement from anything on this host: lz4's `NEWS` never mentions `versionString` and the 1.9.3 header carries no `since` annotation, so I am taking 1.7.5 from you and your 1.10.0 header. It costs nothing on any supported distro, so I would rather have it wrong-and-conservative than be right by accident. `find_package(LZ4 1.7.5)` reports `found suitable version "1.9.3", minimum required is "1.7.5"`, and raising it to 99.0.0 correctly refuses. ## 5. Fatal message — now names the record and the value ``` proxy.config.cache.ram_cache.compress is 5 (zstd), but this build has no zstd support proxy.config.cache.ram_cache.compress has unknown value 7 ``` ## 6. Smaller things - `NOTICE` is byte-identical to master again. The stray newline was mine, from removing the VTK attribution when the modules stopped being derived work. - `ci/docker/yum/Dockerfile` reverted to `zstd-devel`; only the `lz4-devel` addition remains. Worth flagging separately though, because I think it is a real bug rather than just an unrelated change: there is no `zstd-devel` on EL9 and `libzstd-devel` does not provide it, so that line looks like it cannot install as written. It came in with #12201, predates this PR, and belongs in its own change. - `it mat be compressed` → `may`. - `traffic_layout --versions` now prints `lz4` as the runtime version and drops `lz4.run`, so `lz4` and `zstd` mean the same thing. I deliberately did not change what `zstd` reports, since that key has meant runtime since #12201 and someone may be parsing it. ## cmcfarlen's second question It was answered, just not anywhere you would find it: [this comment](https://github.com/apache/trafficserver/pull/13257#issuecomment-3049270861) from 2026-07-08, about fourteen months up the thread and before the rebase. Short version is that this PR does not resolve the CLFUS concerns, and the intent is to refactor so compression is available to any RAM cache algorithm. I will leave it to @phongn whether that wants restating for @cmcfarlen directly. ## On CI coverage Agreed, and it is the reason I keep reporting local numbers. This host has lz4 1.9.3 and zstd 1.5.5, so all six backends run: 131 assertions now, up from 79, and 59 in a `CMAKE_DISABLE_FIND_PACKAGE_*` build. ASan clean, both full-tree builds clean, `clang-analyzer-*` and `.clang-tidy-ci` clean on every file I touched. apache/trafficserver-ci#441 is still what closes the gap for the Fedora image. -- 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]
