Skip to content

Commit 5fb30b4

Browse files
Vrishal Kulkarnimeta-codesync[bot]
authored andcommitted
Preserve client overrides in Luna comparison
Summary: Pass explicit client-owned option names to `McrouterOptionsBase::compare()` so Luna comparison skips only `config_params` and `router_name` overrides while preserving missing-key validation for every Luna-managed option. Apply both overrides as typed fields after Luna materialization, avoiding delimiter-based serialization. Review context This child responds to the following comment from Anton (`alikhtarov`) on D113813215: > Can we reorder the logic here to avoid hasConfigParamsOverride? I.e. > > 1) construct lunaOptions (copy config_params from before) > 2) apply luna settings > 3) compare lunaOptions with currentOptions (need to rewrite the comparison function) > > This is probably more correct as well as we're comparing actual options struct instead of serialized representations. What this addresses: - Removes the `hasConfigParamsOverride` flag and post-comparison `remove_if`; client-owned options are represented explicitly. - Keeps overridden `config_params` and `router_name` out of raw Luna comparison and overlays them as typed fields after Luna application. - Retains existing missing-key errors for every non-overridden Luna-managed option. What this does not address: - It does not reorder the entire pipeline to apply Luna before comparison. - It does not compare two `McrouterOptions` structs; non-overridden options still use the existing raw Luna-map comparison. This narrower scope avoids introducing generic typed-comparison infrastructure and changing scalar normalization semantics while addressing the override-handling concern that motivated the comment. Reviewed By: alikhtarov Differential Revision: D114116126 fbshipit-source-id: 1e0ed10bfe3fa990fb8881e2a44e6c7d2d7b4f9b
1 parent ccfd256 commit 5fb30b4

2 files changed

Lines changed: 9 additions & 3 deletions

File tree

mcrouter/options.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -270,12 +270,16 @@ vector<McrouterOptionError> McrouterOptionsBase::updateFromDict(
270270
}
271271

272272
vector<McrouterOptionMismatch> McrouterOptionsBase::compare(
273-
const std::unordered_map<std::string, std::string>& new_opts) const {
273+
const std::unordered_map<std::string, std::string>& new_opts,
274+
const std::unordered_set<std::string>& ignored_opts) const {
274275
vector<McrouterOptionMismatch> errors;
275-
forEach([&errors, &new_opts](
276+
forEach([&errors, &new_opts, &ignored_opts](
276277
const string& name,
277278
McrouterOptionData::Type type,
278279
const boost::any& value) {
280+
if (ignored_opts.contains(name)) {
281+
return;
282+
}
279283
auto it = new_opts.find(name);
280284
if (it == new_opts.end()) {
281285
McrouterOptionMismatch e;

mcrouter/options.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <functional>
1111
#include <string>
1212
#include <unordered_map>
13+
#include <unordered_set>
1314
#include <vector>
1415

1516
#include <boost/any.hpp>
@@ -84,7 +85,8 @@ class McrouterOptionsBase {
8485
const std::unordered_map<std::string, std::string>& new_opts);
8586

8687
virtual std::vector<McrouterOptionMismatch> compare(
87-
const std::unordered_map<std::string, std::string>& new_opts) const;
88+
const std::unordered_map<std::string, std::string>& new_opts,
89+
const std::unordered_set<std::string>& ignored_opts = {}) const;
8890

8991
virtual ~McrouterOptionsBase() {}
9092

0 commit comments

Comments
 (0)