You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "chenBright (via GitHub)" <gi...@apache.org> on 2026/10/15 03:07:08 UTC
[PR] Fix flaky histogram window ut (brpc)
chenBright opened a new pull request, #3611:
URL: https://github.com/apache/brpc/pull/3611
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
### What is changed and the side effects?
Changed:
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3611:
URL: https://github.com/apache/brpc/pull/3611#discussion_r4305355206
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -734,11 +735,38 @@ TEST_F(HistogramTest, window_forgets_old_samples) {
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
bvar::Window<bvar::Histogram> w(&h, 1);
h << 5;
- sleep(2);
- // The value recorded 2 seconds ago has left the 1-second window.
- ASSERT_EQ(0, w.get_value().num);
+
+ // First wait for the sampler to observe the write. An empty window before
+ // the first sampling round must not count as forgetting the sample. Allow
+ // up to 30 seconds at 100 ms per retry on busy test hosts.
+ bvar::Histogram::Value wv;
+ for (int i = 0; i < 300; ++i) {
+ wv = w.get_value();
+ if (wv.num == 1) {
Review Comment:
[replied by brpc-oncall robot] This phase depends on observing a value that only exists between two consecutive sampling rounds: with window_size == 1, `get_value(1)` is `latest - bottom(1)`, i.e. the counts recorded between the last two snapshots, so `wv.num == 1` lasts exactly one sampling interval. When the sampler thread falls behind (the very busy-host case this PR targets), SamplerCollector does not sleep until it catches up (see src/bvar/detail/sampler.cpp: `abstime += 1000000L` per iteration and the `while (abstime > now)` sleep is skipped while behind), so several rounds can run back to back microseconds apart. If the write lands between two of those catch-up rounds, the `wv.num == 1` state disappears long before the next 100 ms poll, and the test fails on `ASSERT_EQ(1, wv.num)` after spinning for 30 s. Could the check for "the sampler has seen the write" be based on something non-transient, or could the window be enlarged (e.g. a few seconds) so the visible state spans m
ore than one sampling interval and is far more likely to be observed?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3611:
URL: https://github.com/apache/brpc/pull/3611
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3611:
URL: https://github.com/apache/brpc/pull/3611#discussion_r4313928971
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -707,9 +707,10 @@ TEST_F(HistogramTest, window) {
// Window values are produced by the global sampler thread. Wait until it
// observes all writes instead of assuming a fixed sleep covers a sampling
- // round, which is not guaranteed when the test host is busy.
+ // round, which is not guaranteed when the test host is busy. Allow up to
+ // 30 seconds at 100 ms per retry.
bvar::Histogram::Value wv;
- for (int i = 0; i < 50; ++i) {
+ for (int i = 0; i < 300; ++i) {
wv = w.get_value();
if (wv.num == 3) {
break;
Review Comment:
The full loop already calls `usleep(100 * 1000)` after each unsuccessful check at line 718. It allows 300 retries with a 100 ms sleep, while exiting immediately on success. The sleep is just outside the displayed diff hunk.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3611:
URL: https://github.com/apache/brpc/pull/3611#issuecomment-6306516521
@wwbmmm PTAL.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3611:
URL: https://github.com/apache/brpc/pull/3611#discussion_r4307363193
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -734,11 +736,46 @@ TEST_F(HistogramTest, window_forgets_old_samples) {
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
bvar::Window<bvar::Histogram> w(&h, 1);
h << 5;
- sleep(2);
- // The value recorded 2 seconds ago has left the 1-second window.
- ASSERT_EQ(0, w.get_value().num);
+
+ // Check the latest cumulative snapshot: it still includes the write even
+ // if the one-second window has expired before this thread can poll it.
+ // Allow up to 30 seconds at 100 ms per retry on busy test hosts.
+ bvar::Histogram::sampler_type* sampler = h.get_sampler();
+ int64_t sampled_count = 0;
+ for (int i = 0; i < 300; ++i) {
+ {
+ BAIDU_SCOPED_LOCK(sampler->_mutex);
+ const auto* latest = sampler->_q.bottom();
+ if (latest != nullptr) {
+ sampled_count = latest->data.num;
+ }
+ }
+ if (sampled_count == 1) {
+ break;
+ }
+ usleep(100 * 1000);
+ }
+ ASSERT_EQ(1, sampled_count);
Review Comment:
The cumulative snapshot alone did not prove that the window returned a non-zero value.
I've changed this test to use an unscheduled sampler and advance sampling manually in
6f848ba. It now verifies that the same window returns `1` after the first round and `0` after the next, including the expected sum and bucket counts. This removes the dependency on thread scheduling. The asynchronous window test retains its 30-second timeout.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3611:
URL: https://github.com/apache/brpc/pull/3611#discussion_r4306865297
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -734,11 +736,46 @@ TEST_F(HistogramTest, window_forgets_old_samples) {
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
bvar::Window<bvar::Histogram> w(&h, 1);
h << 5;
- sleep(2);
- // The value recorded 2 seconds ago has left the 1-second window.
- ASSERT_EQ(0, w.get_value().num);
+
+ // Check the latest cumulative snapshot: it still includes the write even
+ // if the one-second window has expired before this thread can poll it.
+ // Allow up to 30 seconds at 100 ms per retry on busy test hosts.
+ bvar::Histogram::sampler_type* sampler = h.get_sampler();
+ int64_t sampled_count = 0;
+ for (int i = 0; i < 300; ++i) {
+ {
+ BAIDU_SCOPED_LOCK(sampler->_mutex);
+ const auto* latest = sampler->_q.bottom();
+ if (latest != nullptr) {
+ sampled_count = latest->data.num;
+ }
+ }
+ if (sampled_count == 1) {
+ break;
+ }
+ usleep(100 * 1000);
+ }
+ ASSERT_EQ(1, sampled_count);
Review Comment:
This phase only proves that the underlying sampler produced a cumulative snapshot (`latest->data.num == 1`); it never verifies that the window observed a non-zero value before the expiration phase. A regression where `w.get_value()` always returned an empty histogram would therefore pass immediately through the next loop and satisfy the final zero assertions. Poll/assert the window's `num == 1` before waiting for it to become zero, while retaining the timeout coordination needed to avoid missing that sample under load.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3611:
URL: https://github.com/apache/brpc/pull/3611#discussion_r4306793567
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -734,11 +735,38 @@ TEST_F(HistogramTest, window_forgets_old_samples) {
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
bvar::Window<bvar::Histogram> w(&h, 1);
h << 5;
- sleep(2);
- // The value recorded 2 seconds ago has left the 1-second window.
- ASSERT_EQ(0, w.get_value().num);
+
+ // First wait for the sampler to observe the write. An empty window before
+ // the first sampling round must not count as forgetting the sample. Allow
+ // up to 30 seconds at 100 ms per retry on busy test hosts.
+ bvar::Histogram::Value wv;
+ for (int i = 0; i < 300; ++i) {
+ wv = w.get_value();
+ if (wv.num == 1) {
Review Comment:
The first phase could indeed miss the transient `num == 1` value.
I've changed it to check the sampler's latest cumulative snapshot under its mutex in 15692d5. This confirms the write has been sampled even if the window has already expired. Both polling phases retain their 30-second timeout.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3611:
URL: https://github.com/apache/brpc/pull/3611#discussion_r4313676751
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -731,14 +732,33 @@ TEST_F(HistogramTest, window) {
}
TEST_F(HistogramTest, window_forgets_old_samples) {
+ // Own the unscheduled sampler until after h's destructor calls destroy().
+ std::unique_ptr<bvar::Histogram::sampler_type> sampler;
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
+ sampler = std::make_unique<bvar::Histogram::sampler_type>(&h);
Review Comment:
`std::make_unique` requires C++14; if this repo/tests are compiled as C++11 (common for older setups), this will fail to compile. If C++14 isn’t guaranteed here, replace `std::make_unique<...>(&h)` with an equivalent C++11 construction (e.g., `std::unique_ptr<...>(new ...)`) or use an existing project helper that provides `make_unique` on C++11.
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -707,9 +707,10 @@ TEST_F(HistogramTest, window) {
// Window values are produced by the global sampler thread. Wait until it
// observes all writes instead of assuming a fixed sleep covers a sampling
- // round, which is not guaranteed when the test host is busy.
+ // round, which is not guaranteed when the test host is busy. Allow up to
+ // 30 seconds at 100 ms per retry.
bvar::Histogram::Value wv;
- for (int i = 0; i < 50; ++i) {
+ for (int i = 0; i < 300; ++i) {
wv = w.get_value();
if (wv.num == 3) {
break;
Review Comment:
The comment says “100 ms per retry” but this hunk doesn’t show any sleep/backoff in the retry loop; if there isn’t a sleep/yield elsewhere in the loop body, this becomes a tight busy-spin that both wastes CPU and may still not “wait 30 seconds” in wall time. Add an explicit 100ms sleep/yield on unsuccessful iterations (or update the comment if the sleep is implemented below these lines).
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -731,14 +732,33 @@ TEST_F(HistogramTest, window) {
}
TEST_F(HistogramTest, window_forgets_old_samples) {
+ // Own the unscheduled sampler until after h's destructor calls destroy().
+ std::unique_ptr<bvar::Histogram::sampler_type> sampler;
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
+ sampler = std::make_unique<bvar::Histogram::sampler_type>(&h);
+ h._sampler = sampler.get();
Review Comment:
This test now directly assigns to `h._sampler`, which tightly couples it to `bvar::Histogram` internals and makes the test brittle if the implementation changes. Consider wrapping this in a small test-only RAII helper (in this test file) that “installs” an explicit sampler and documents required invariants/cleanup, or prefer a public/test-supported hook if one exists, so future refactors don’t silently break the unit test.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3611:
URL: https://github.com/apache/brpc/pull/3611#discussion_r4313933385
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -731,14 +732,33 @@ TEST_F(HistogramTest, window) {
}
TEST_F(HistogramTest, window_forgets_old_samples) {
+ // Own the unscheduled sampler until after h's destructor calls destroy().
+ std::unique_ptr<bvar::Histogram::sampler_type> sampler;
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
+ sampler = std::make_unique<bvar::Histogram::sampler_type>(&h);
+ h._sampler = sampler.get();
Review Comment:
The current setup already uses `std::unique_ptr` for ownership, and declaration order ensures destruction proceeds as window → histogram → sampler. The sampler remains alive through the histogram's destructor, including when an assertion fails.
Since this setup is used by a single white-box test, I suggest keeping it local for now and extracting a helper if additional tests need it.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3611:
URL: https://github.com/apache/brpc/pull/3611#issuecomment-6267515720
@wwbmmm The first two Copilot comments are false positives, and the third is an optional maintainability suggestion. No required fixes remain. This PR is ready to merge
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix flaky histogram window ut (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3611:
URL: https://github.com/apache/brpc/pull/3611#discussion_r4313930894
##########
test/bvar_histogram_unittest.cpp:
##########
@@ -731,14 +732,33 @@ TEST_F(HistogramTest, window) {
}
TEST_F(HistogramTest, window_forgets_old_samples) {
+ // Own the unscheduled sampler until after h's destructor calls destroy().
+ std::unique_ptr<bvar::Histogram::sampler_type> sampler;
bvar::Histogram h(bvar::Histogram::BucketSchema({10, 20, 30}));
+ sampler = std::make_unique<bvar::Histogram::sampler_type>(&h);
Review Comment:
The repository already requires C++14 or newer. CMake propagates this requirement to the test targets, Make uses C++14/C++17, and Bazel uses C++17. Therefore, `std::make_unique` is supported by the current build configurations.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org