Adar Dembo has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 25:


I actually reviewed the changes to client-test this time.
File src/kudu/client/

PS25, Line 182:   // Must be public to use as a thread closure.
They can be static functions, though, right? I don't see them accessing any 
test state.

PS25, Line 187: Microseconds
How did you choose 10 us? It seems low to me (i.e. maybe we can get away with a 
higher granularity), but maybe I'm missing something.

PS25, Line 199:   
Nit: extra space here.

PS25, Line 553: oprations
Nit: operations

PS25, Line 556: WaitForAutoFlushBackground
Hmm, when this returns, there's no actual guarantee that a batch flushed, 
right? We may have just expired the deadline?

PS25, Line 558: MAX_WAIT_MS

Line 1207:         << "unexpected status: " << result.ToString();
There's a better technique for handling situations like these:

  Status s = DoSomething();

If a test fails while SCOPED_TRACE() is still in scope, gtest will print 
s.ToString(). That way you don't have to worry about printing it yourself 
following every potential ASSERT/EXPECT that may fail.

PS25, Line 2153: BUFFER_SIZE
kBufferSizeBytes (camel case, k prefix, and append the unit)

Below too. Please fix all of your "constants" to follow this naming convention.

PS25, Line 2186: // Applying a big operation which does not fit into the buffer 
               : // return an error with session running in any supported flush 
Doesn't this test make the second half of TestApplyTooMuchWithoutFlushing 

PS25, Line 2197: for (auto mode: FLUSH_MODES)
Nit: auto mode : FLUSH_MODES

(separate 'mode' from the colon with a space)

PS25, Line 2215:     STLDeleteElements(&errors);
Nit: use an ElementDeleter and declare it right after declaring 'errors'. Then 
there's no possibility for a leak.

PS25, Line 2258:   WaitForAutoFlushBackground(session, FLUSH_INTERVAL_MS);
               :   // There should be some pending ops: the automatic 
background flush
               :   // should not be active after switch to the MANUAL_FLUSH 
I see. How expensive is this in terms of test runtime? You've dropped the flush 
interval to 100 ms; how much time do we end up waiting here?

Line 2304:     *result = t_end - t_begin;
Have you looked at the StopWatch class? May be easier for this kind of thing.

PS25, Line 2335: // Check that call to Flush()/FlushAsync() is no-op if there 
isn't current
               : // batcher for the session.
Doesn't this test make TestEmptyBatch redundant?

PS25, Line 2348:   ASSERT_OK(sync0.Wait());
               :   // The same with the synchronous flush.
               :   ASSERT_OK(session->Flush());
               :   ASSERT_FALSE(session->HasPendingOperations());
               :   ASSERT_EQ(0, session->CountPendingErrors());
How does this verify that flush was a no-op? Seems to me you need to find some 
metrics indicating that no Write RPCs were sent.

PS25, Line 2391: kudu::Thread::Create
Now that we're building with C++11, we prefer to use std::thread for test 
threads. There's less boilerplate with them (i.e. no need to make up a thread 
category and name), and they have better support for lambdas, which I think you 
can use here pretty effectively.

Same goes for the other new uses of kudu::Thread.

Line 2428:   // AUTO_FLUSH_BACKGROUND should be faster than AUTO_FLUSH_SYNC.
Hmm, is this expected to be true 100% of all test runs? Even with debug 
instrumentation, TSAN, ASAN, etc? Wondering if this test will be flaky.

A good way to find out is to use build-support/ to loop your test 
1000 times. You can use --gtest_filter on the command line to run just this one 
test case (as opposed to all of client-test, much of which is already flaky).

Line 2429:   EXPECT_GT(t_diff_afs.ToMilliseconds(), 
Can't we just EXPECT_GT on the MonoDeltas directly now?

PS25, Line 2451:   // AUTO_FLUSH_BACKGROUND should be faster than 
               :   EXPECT_GT(t_diff_afs.ToMilliseconds(), 
Same issues and questions as in the above test.

PS25, Line 2456: // applying a bunch of small rows without a flush should not 
result in
               : // an error, even with low limit on the buffer space.  This is 
               : // Session::Apply() blocks and waits for buffer space to 
become available.
For this test, you may want to scan once you're done writing to make sure that 
all of the rows really made it and that Apply() didn't just decide to throw 
some rows away because we ran out of buffer space.

Line 2488: // applying a bunch of rows every of which is so big in size that
Nit: every one of which

PS25, Line 2489: does
Nit: do

PS25, Line 2510: 3
Why does i start at 3 and not 0?

PS25, Line 2574:   ASSERT_TRUE(session->HasPendingOperations());
This is the first test where I've noticed this but others may be affected too.

It's risky to have this assert when using background flushing. We've seen Java 
tests become flaky from asserting this. Scheduling delays may cause the 
background flush to be executed before this assert runs, especially with a low 
interval like 100ms.

If you want your test to be robust, it should either:
1. Properly handle the case where HasPendingOperations() has already become 
false (i.e. short-circuit out of the test, I guess).
2. Loop a bunch of times until at least one run works as expected, though I 
don't know if this is actually more robust in slow environments like TSAN.
3. Use a much higher flush interval, though this has the side effect of 
increasing test runtime, potentially significantly.

Again, when in doubt, use to loop the test 1000 times in a 
pathological environment (e.g. TSAN) and see whether it fails.

PS25, Line 2612:     const MonoTime t_begin(MonoTime::Now());
               :     ASSERT_OK(session->Apply(insert.release()));
               :     const MonoTime t_end(MonoTime::Now());
Consider StopWatch for this.

PS25, Line 2627:     // It's supposed that sending an operation to the server 
at least
               :     // order of magnitude less than the periodic flush 
Nit: "Sending an operation to the server is supposed to be at least an order of 
magnitude less than the periodic flush interval"

PS25, Line 2630:     EXPECT_GT(FLUSH_INTERVAL_MS / 10, t_diff.ToMilliseconds());
Again, worried about flakiness due to testing timing like this.

PS25, Line 2634: It's supposed the flushed operations
               :   // reach the tablet server faster than the max-wait flush 
Nit: "The flushed operations should reach the tablet server faster than the 
max-wait flush interval."

PS25, Line 2636:   WaitForAutoFlushBackground(session, FLUSH_INTERVAL_MS);
This will be at least a 10s pause, right? If so, can you condition the entire 
test on AllowSlowTests()?

PS25, Line 2645: accomodate
Nit: accommodate

PS25, Line 2653: gurantees
Nit: guarantees

PS25, Line 2663: TestAutoFlushBackgroundApplyBlocks
Lots of opportunities for flakiness here due to timing measurements, please 
test with dist-test and TSAN/ASAN.

PS25, Line 2714: It's supposed the flushed operations
               :   // reach the tablet server faster than wait_timeout_ms.
Nit: reword as per the above.
File src/kudu/client/client.h:

PS25, Line 1351: successfull

PS25, Line 1352: left
be left
File src/kudu/client/

PS23, Line 296: 
              :     if (batcher_) {
              :       const MonoTime f
> OK, I see.  I suspected something like that, but since I haven't seen anyth
This means acquiring a Mutex on the hot path, though. Hmm. The implementation 
does a TryAcquire() in Acquire() so that it will do just a CAS when 
uncontended, so maybe that's OK.

May want to ask Todd about this one (that is, about whether taking a lock is 
File src/kudu/client/

PS25, Line 106: PRCs

PS25, Line 150: PRCs

PS25, Line 174: PRCs

PS25, Line 208: PRCs
File src/kudu/client/session-internal.h:

PS25, Line 102: NoLock
Kudu convention is to use an "Unlocked" suffix for methods like these.

PS25, Line 144: This method is used by tests only.
Kudu convention is to add the suffix "ForTests" for test-only methods like 
these (and below).

PS25, Line 151: is here to represent
Nit: just "represents"

PS25, Line 152: // the first argument in expressions like 
FlushCurrentBatcher(1, cbk):
              :   // this is the watermark corresponding to 1 byte of data.
Yes, but what is the effect? What is special about choosing a value of 1?

PS25, Line 181:   // Note that this mutex should not be acquired if the thread 
is already
              :   // holding a Batcher's lock. This must be acquired first.
How is such sequencing possible? Isn't the batcher's lock internal to the 
batcher, and thus even if you call a batcher method that acquires the batcher 
lock, it is released before the call returns?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 25
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to