cmcfarlen commented on PR #13493: URL: https://github.com/apache/trafficserver/pull/13493#issuecomment-5196110740
The core change looks right. Converting `initialize()` failure into an exception routes it into the existing `catch (std::runtime_error &)` at `header_rewrite.cc:413`, so the rule is rejected at config load time and a reload simply keeps the running config. The `unique_ptr` conversions are real leak fixes rather than style: the old `RuleSet::add_operator` depended on an explicit `delete` on the `is_hook_valid` failure path, and the `if_stack` unwinding loop becomes unnecessary rather than merely correct. Replacing the `TSReleaseAssert` in `exec()` with a guarded early return is sound belt-and-braces, and returning `true` is the right choice since remap pseudo-hook operators must return true (`operator.h:92-94`). **One thing to flag for the 10.2.0 backport.** This PR is tracked at `For v10.2.0` in the [ATS v10.2.x project](https://github.com/orgs/apache/projects/573), but the new test calls `Test.AddConfigReload()`, which lives in `tests/gold_tests/autest-site/config_reload.test.ext` — a file that exists only on master. It is not on 10.2.x, so the test would fail to load there with `AttributeError`-style breakage at test collection time. The good news is that this looks cheap to resolve, because only the autest extension is missing — every runtime dependency it needs is already on 10.2.x: - `traffic_ctl config reload` already supports `--monitor/-m`, `--token/-t`, `--initial-wait/-w`, and `--refresh-int/-r` (`src/traffic_ctl/traffic_ctl.cc:135-173`). - The `get_reload_config_status` RPC handler is present (`src/mgmt/rpc/handlers/config/Configuration.cc:384`, registered in `RpcAdminPubHandlers.cc:43`). - `jsonrpc.py` resolves `Request.get_reload_config_status` dynamically via `__getattr__`, so it needs no explicit addition, and the `CustomJSONRPCResponse` tester already exists in `jsonrpc_client.test.ext:164`. So the extension file should drop onto 10.2.x and work. The wrinkle is that the commit which introduced it, #13075 (`70859231d8`), is a 31-file change that also converts ~20 existing tests to the new helper and deletes `wait_reload.sh` — more churn than I would want to land on a release branch this close to the RC. Picking just `tests/gold_tests/autest-site/config_reload.test.ext` from that commit, without the test conversions, would give 10.2.x the API this test needs while leaving the existing passing tests alone. Happy to do that as a small prerequisite pick if you and the RM agree that is the right shape. -- 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]
