Copilot commented on code in PR #12593:
URL: https://github.com/apache/trafficserver/pull/12593#discussion_r4272422224
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -114,6 +114,14 @@ NextHopStrategyFactory::NextHopStrategyFactory(const char
*file) : fn(file)
}
done:
+ if (strategies_loaded) {
+ // Precomputed so that strategyInstance() stays read-only.
+ uint32_t idx = 0;
+ for (auto &[name, strat] : _strategies) {
+ strat->distance = idx++;
+ _strategy_ptrs.insert(strat);
+ }
+ }
Review Comment:
`distance` is being assigned based on `std::unordered_map` iteration order,
which is not deterministic across platforms/builds and can change if
hashing/rehashing behavior changes. If `distance` is intended to be *stable*
(as suggested by the new unit test wording), consider assigning it
deterministically (e.g., store an insertion-ordered list when parsing the YAML,
or assign indices by sorting strategy names). If stability isn’t required, it
would be better to avoid implying it via naming/comments/tests.
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -58,6 +58,25 @@ SCENARIO("factory tests loading yaml configs",
"[loadConfig]")
}
}
+ WHEN("strategy distances are checked.")
+ {
+ THEN("each strategy has a unique, stable index and lookups do not mutate
it")
+ {
+ NextHopSelectionStrategy *const s1 =
nhf.strategyInstance("strategy-1");
+ NextHopSelectionStrategy *const s2 =
nhf.strategyInstance("strategy-2");
+ REQUIRE(s1 != nullptr);
+ REQUIRE(s2 != nullptr);
+
+ CHECK(s1->distance != s2->distance);
+ CHECK(s1->distance < 2);
+ CHECK(s2->distance < 2);
+
+ const uint32_t d1 = s1->distance;
+ nhf.strategyInstance("strategy-1");
+ CHECK(s1->distance == d1);
+ }
Review Comment:
The assertions `distance < 2` hard-code the number of strategies in the test
config and will break if the fixture YAML grows beyond two strategies. Consider
removing those checks (the `!=` assertion already verifies uniqueness for the
strategies under test) or replacing them with a bound derived from the
factory/config (e.g., a factory accessor for strategy count, if available).
--
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]
