[Nexthop][fsdb] drop duplicate gflags DEFINE stat_publish_interval_ms - #1435
Open
saty-nexthop wants to merge 1 commit into
Open
[Nexthop][fsdb] drop duplicate gflags DEFINE stat_publish_interval_ms#1435saty-nexthop wants to merge 1 commit into
saty-nexthop wants to merge 1 commit into
Conversation
fboss/fsdb/server/Server.cpp DEFINEs stat_publish_interval_ms with the same name, default and help text as fboss/agent/AgentFeatures.cpp, and Server.h DECLAREs it. Nothing under fboss/fsdb reads FLAGS_stat_publish_interval_ms -- the only readers are agent side (HwAgentMain.cpp, SwAgentInitializer.cpp) and they get the flag from AgentFeatures. The fsdb copy is dead. It is also a latent hard failure. gflags treats two DEFINEs of the same flag name coming from different files as fatal: ERROR: flag 'stat_publish_interval_ms' was defined more than once (in files '...' and '...'). That is reported with DIE, from the FlagRegisterer constructor, so a binary holding both definitions aborts during static initialization, before main() ever runs. fsdb does not link agent_features today, so the duplicate has stayed dormant, but any build that ends up with both objects in the fsdb binary turns it into a startup crash loop that is awkward to diagnose. Remove the dead fsdb DEFINE/DECLARE. The agent side definition, which is also the flag's only consumer, remains.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
fboss/fsdb/server/Server.cppdefinesstat_publish_interval_ms, andfboss/agent/AgentFeatures.cppdefines the same flag. Same name, same default, same help string. fsdb never reads it though. The only readers areHwAgentMain.cppandSwAgentInitializer.cpp, and both pick it up fromAgentFeatures.That stays harmless right up until something links both objects into one binary. gflags hits
ReportError(DIE, ...)on a secondDEFINEof a flag it already has from a different file, and it does that inside theFlagRegistererconstructor, so the process dies in static init and never reachesmain(). fsdb doesn't linkagent_featurestoday, so nobody has tripped it, but it's a trap for anything that touches fsdb's link line.So drop the fsdb
DEFINE/DECLARE. The agent copy stays, and it's the only one anyone reads.One caveat worth flagging: fsdb parses with
gflags::ParseCommandLineFlags, so--stat_publish_interval_mson an fsdb command line is now an unknown-flag error. Nothing in the tree passes it to fsdb, and the flag did nothing for fsdb anyway, but if a config somewhere outside the repo sets it, that needs to go.Test Plan
Grepped the flag after the change: four hits left, all agent side, none under
fboss/fsdb. Built the cmakefsdbtarget, compiles and links.pre-commit runclean on both files.