[docdb] Make gflag update safe in production #5892
Assignees
Labels
area/docdb
YugabyteDB core features
kind/enhancement
This is an enhancement of an existing feature
priority/medium
Medium priority issue
Comments
bmatican
added a commit
that referenced
this issue
Oct 1, 2020
Summary: Since we should never write to gflags from the main code, we might as well ignore all test race conditions on gflags. Currently, the main DB code should not be updating gflags, so all race conditions we find in testing should be strictly due to tests writing, while the server side code does reads. We should be safe to ignore those! There are two caveats to this: - Despite test-only, race conditions on updating `string` gflags can still cause issues, and technically, a TSAN report on those races would be easier to figure out, than say, a SEGV - For production code, while the DB itself does not normally update gflags, we do have a `yb-ts-cli set_flag` command, through which we can runtime update flags, in memory. We frequently use that, for modifying various settings, without restarting servers. While the RPC itself uses `SetAtomic`, the rest of the code does not, so that use case can generate are legitimate races! Have filed #5892 for fixing that in the future. Test Plan: jenkins and also tried out on a test that would give races at a 1/100 rate: `ybd tsan --cxx-test client_backup-txn-test --gtest_filter BackupTxnTest.DeleteTable -n 500` Reviewers: sergei, timur, mikhail Reviewed By: mikhail Subscribers: ybase Differential Revision: https://phabricator.dev.yugabyte.com/D9454
yugabyte-ci
added
kind/bug
yugabyte-ci
assigned rthallamko3 and lingamsandeep and unassigned bmatican and rthallamko3
yugabyte-ci
added
kind/enhancement
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Jira Link: DB-2070
Currently, we have a
yb-ts-cli set_flagcommand that we use to update gflags in production, without restarting servers. Some gflags are runtime safe that way, allowing us to dynamically enable/disable certain features, or tweak various parameters.However, while
set_flaguses aSetAtomicprimitive for updating the gflags, the rest of the code does not use the corresponding concurrency safeGetAtomic, thus leaving us with race conditions in prod, when updating flags this way!We should fix that, either by patching the gflag library itself somehow, or at least introducing a macro to be used across the codebase, when reading gflags, which uses
GetAtomicunder the hood.cc @mbautin @spolitov @robertsami
Note: this is the proper fix over the short term, test-only #5800
The text was updated successfully, but these errors were encountered: