airborne12 commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4079903845
##########
be/src/runtime/index_policy/index_policy_mgr.cpp:
##########
@@ -41,13 +43,72 @@ class SingleAnalyzerProvider final : public
segment_v2::inverted_index::Analyzer
const std::unordered_set<std::string> IndexPolicyMgr::BUILTIN_NORMALIZERS =
{"lowercase"};
-std::string IndexPolicyMgr::normalize_name(const std::string& name) {
+std::string IndexPolicyMgr::trim_name(const std::string& name) {
std::string result = name;
boost::algorithm::trim(result);
+ return result;
+}
+
+std::string IndexPolicyMgr::normalize_name(const std::string& name) {
+ std::string result = trim_name(name);
boost::algorithm::to_lower(result);
return result;
}
+const TIndexPolicy* IndexPolicyMgr::find_policy_by_name_locked(const
std::string& name) const {
+ const std::string exact_name = trim_name(name);
+ if (auto exact_it = _exact_name_to_id.find(exact_name); exact_it !=
_exact_name_to_id.end()) {
Review Comment:
Thanks - the mechanism you describe is real, but we do not think it is worth
carrying a persisted binding for, and we would like to explain why rather than
just close this.
The state the scenario needs is two policies whose names differ only in
case. `IndexPolicyMgr.createIndexPolicy()` rejects that: it looks up the
normalized name and throws `Index policy <name> already exists`. That check is
not new here - it is already on the base this PR targets, and it has been there
since #59117. So no cluster on a current version can create the pair through
DDL, and the BE unit test builds it by replaying policies directly, to pin
resolution determinism rather than to model a reachable cluster state.
The only opening is a cluster that created such a pair before #59117 landed
and still holds it. We are treating that as out of scope: the names would have
to differ only in case, an index would have to be bound to the exact spelling,
and that data would have to predate the check.
There is also no well-defined base binding to preserve for that state. On
the base, resolution went through `_name_to_id.emplace(normalized_name, id)`,
so the first policy to arrive won, and the list it sees comes from
`getCopiedIndexPolicies()`, which returns `idToIndexPolicy.values()` of a
`HashMap`. The binding for a colliding pair therefore had no ordering contract
on the base either, and a policy re-push could already change which component
an index resolved to, without any upgrade.
Finally, the exact-name preference this thread points at was added earlier
in this PR at review request, so that an index stays bound to the component its
name actually names. Reverting to normalized-only resolution would reopen that.
For those reasons we are not adding a persisted or versioned per-index
binding, nor the base-manager upgrade test, in this PR. If you think the
pre-#59117 window is reachable in a way we have missed, or you have a concrete
cluster shape where this bites, we are happy to look again.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
