Copilot commented on code in PR #12593:
URL: https://github.com/apache/trafficserver/pull/12593#discussion_r4484484684
##########
include/proxy/http/remap/NextHopStrategyFactory.h:
##########
@@ -50,11 +51,16 @@ class NextHopStrategyFactory
// counted and attached to an HttpSM.
NextHopSelectionStrategy *strategyInstance(const char *name) const;
+ // Pointer comparison only; safe to call with a dangling pointer.
+ bool contains(NextHopSelectionStrategy const *strategy) const;
+
bool strategies_loaded;
private:
std::string fn;
void loadConfigFile(const std::string &file, std::stringstream &doc,
std::unordered_set<std::string> &include_once);
void createStrategy(const std::string &name, const NHPolicyType
policy_type, ts::Yaml::Map &node);
std::unordered_map<std::string, NextHopSelectionStrategy *> _strategies;
+ std::unordered_set<NextHopSelectionStrategy const *> _strategy_ptrs;
Review Comment:
`std::unordered_set` is used for `_strategy_ptrs` but `<unordered_set>` is
not included in this header. This can fail to compile depending on transitive
includes; add an explicit `#include <unordered_set>` here.
##########
include/proxy/http/remap/NextHopStrategyFactory.h:
##########
@@ -23,6 +23,7 @@
#pragma once
+#include <atomic>
#include <memory>
#include <sstream>
#include <unordered_map>
Review Comment:
`std::unordered_set` is used for `_strategy_ptrs` but `<unordered_set>` is
not included in this header. This can fail to compile depending on transitive
includes; add an explicit `#include <unordered_set>` here.
##########
include/ts/ts.h:
##########
@@ -1757,18 +1758,40 @@ void TSHttpTxnErrorBodySet(TSHttpTxn txnp, char *buf,
size_t buflength, char *mi
*/
char *TSHttpTxnErrorBodyGet(TSHttpTxn txnp, size_t *buflength, char
**mimetype);
+/**
+ Retrieves a handle to the named strategy in the strategy table.
+ Returns nullptr if no strategy is found.
+ This uses the current transaction's state machine to get
+ access to UrlRewrite's NextHopStrategyFactory.
+ It's preferable to retrieve strategies during TSRemapNewInstance.
+
+ @param txnp HTTP transaction which holds the strategy table.
+ @param name of the strategy to look up.
Review Comment:
The header doc for `TSHttpTxnNextHopStrategyFind` doesn’t mention that
`name` must not be `nullptr` (the implementation asserts via `sdk_assert`).
Consider adding an explicit note here (similar to the standalone `.rst` docs)
so plugin authors aren’t surprised by an abort in debug/default builds.
##########
plugins/lua/ts_lua_http.cc:
##########
@@ -549,6 +552,31 @@ ts_lua_http_get_next_hop_strategy(lua_State *L)
return 1;
}
+namespace
+{
+// A script naming a missing strategy fails on every transaction, so emit at
most one error per interval.
+constexpr TSHRTime strategy_error_interval = TS_HRTIME_SECONDS(60);
+std::atomic<TSHRTime> next_strategy_error{0};
+std::atomic<uint64_t> suppressed_strategy_errors{0};
+
+void
+log_strategy_lookup_failure(char const *name)
+{
+ TSHRTime const now = TShrtime();
+ TSHRTime next = next_strategy_error.load(std::memory_order_relaxed);
+
+ if (now >= next && next_strategy_error.compare_exchange_strong(next, now +
strategy_error_interval, std::memory_order_relaxed)) {
+ uint64_t const suppressed = suppressed_strategy_errors.exchange(0,
std::memory_order_relaxed);
+ TSError("[ts_lua][ts_lua_http_set_next_hop_strategy] Failed get next hop
strategy name '%s' (%" PRIu64
+ " similar errors suppressed)",
+ name, suppressed);
+ } else {
+ suppressed_strategy_errors.fetch_add(1, std::memory_order_relaxed);
+ Dbg(dbg_ctl, "Failed get next hop strategy name '%s'", name);
Review Comment:
The error/debug message grammar is off: \"Failed get\" should be \"Failed to
get\". Updating this string will also require updating any tests that assert on
the exact message (e.g. the uranium `EXPECTED_ERRORS` patterns).
--
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]
