wwbmmm commented on code in PR #3651:
URL: https://github.com/apache/brpc/pull/3651#discussion_r4370301625
##########
test/bvar_recorder_unittest.cpp:
##########
@@ -222,6 +224,19 @@ class LatencyRecorderWithWindowSample : public
bvar::LatencyRecorder {
bool get_latency_sample(bvar::detail::Sample<bvar::Stat>* sample) const {
return _latency_window.get_span(sample);
}
+
+ bool get_latency_sample(time_t window_size,
+ bvar::detail::Sample<bvar::Stat>* sample) const {
+ return _latency_window.get_span(window_size, sample);
+ }
+
+ bool has_full_latency_window() const {
+ std::vector<bvar::Stat> samples;
+ _latency_window.get_samples(&samples);
+ // get_samples() excludes the oldest snapshot used by get_span().
+ return samples.size() + 1 ==
+ static_cast<size_t>(_latency_window.window_size());
Review Comment:
[replied by brpc-oncall robot] I disagree with the review comment on this
line: the helper does not accept a non-full window. On the fallback path
(`_q.bottom(window_size) == nullptr`) `ReducerSampler::get_samples()` sets
`oldest = _q.top()` and the loop breaks at the first snapshot equal to it, so
it appends `size - 2` snapshots, not `window_size - 1`. Concretely, for a 2s
window with 2 snapshots it appends 0 (`0 + 1 != 2`), and with 3 snapshots it
appends 1 (`1 + 1 == 2`). The equality only holds when `bottom(window_size)`
exists, i.e. the queue retains at least `window_size + 1` snapshots, which is
exactly the full 2s/3s span. Verified against `src/bvar/detail/sampler.h` and
`src/bvar/window.h` in the current tree, so no change is needed here.
##########
test/bvar_recorder_unittest.cpp:
##########
@@ -276,44 +291,65 @@ TEST(RecorderTest, latency_recorder_window_statistics) {
}
TEST(RecorderTest, latency_recorder_qps_accuracy) {
- bvar::LatencyRecorder lr1(2); // set windows size to 2s
- bvar::LatencyRecorder lr2(2);
- bvar::LatencyRecorder lr3(2);
- bvar::LatencyRecorder lr4(2);
- usleep(3000000); // wait sampler to sample 3 times
+ LatencyRecorderWithWindowSample lr1_2s(2);
+ LatencyRecorderWithWindowSample lr2_2s(2);
+ LatencyRecorderWithWindowSample lr3_2s(2);
+ LatencyRecorderWithWindowSample lr4_2s(2);
+ LatencyRecorderWithWindowSample lr1_3s(3);
+ LatencyRecorderWithWindowSample lr2_3s(3);
+ LatencyRecorderWithWindowSample lr3_3s(3);
+ LatencyRecorderWithWindowSample lr4_3s(3);
auto write = [](bvar::LatencyRecorder& lr, int times) {
for (int i = 0; i < times; ++i) {
lr << 1;
}
};
- write(lr1, 10);
- write(lr2, 11);
- write(lr3, 3);
- write(lr4, 1);
- usleep(1000000); // wait sampler to sample 1 time
-
- auto read = [](bvar::LatencyRecorder& lr, double exp_qps, int window_size
= 0) {
- int64_t qps_sum = 0;
- int64_t exp_qps_int = (int64_t)exp_qps;
- for (int i = 0; i < 1000; ++i) {
- int64_t qps = window_size ? lr.qps(window_size): lr.qps();
- EXPECT_GE(qps, exp_qps_int - 1);
- EXPECT_LE(qps, exp_qps_int + 1);
- qps_sum += qps;
+ write(lr1_2s, 10);
+ write(lr2_2s, 11);
+ write(lr3_2s, 3);
+ write(lr4_2s, 1);
+ write(lr1_3s, 10);
+ write(lr2_3s, 11);
+ write(lr3_3s, 3);
+ write(lr4_3s, 1);
+
+ auto check_qps = [](LatencyRecorderWithWindowSample& lr, int count,
+ time_t window_size, bool use_default_window) {
+ const int64_t deadline = butil::monotonic_time_us() + 5 * 1000000L;
Review Comment:
[replied by brpc-oncall robot] The condition is satisfiable for only about
one sampler tick, which keeps a residual version of the flakiness this PR is
meant to remove. With only the initial snapshot taken before `write()`, a full
`window_size` window that still contains all `count` writes exists only between
the (`window_size + 1`)-th and (`window_size + 2`)-th snapshot: exactly queue
size 3 for the 2s recorders and size 4 for the 3s recorders (the samplers keep
about `window_size + 2` snapshots). If this thread is delayed by ~1s inside
that interval, the loop spins until the 5s deadline and fails. Priming the
sampler before writing (wait for `has_full_latency_window()` and then
`write()`) would keep the batch inside a full window for more than one tick and
remove most of that risk with a small change.
--
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]
