Adar Dembo has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 25:

File src/kudu/client/

PS25, Line 187: Microseconds
> The idea was to monitor those values as closely as possible.  Without resor
Just to be clear, I meant "coarser" where I wrote "higher" granularity. That 
is, perhaps this could be as large of a value as 10ms, for example.

It just struck me as a very tight loop. Tight loops are useful when trying to 
exercise a data race, but in this case, I would expect a full end-to-end 
Flush() to take on the order of 1ms, especially in a debug or slower build. If 
that's true, checking every 10us is excessive.

But, it shouldn't slow down the tests significantly. If I'm the only one who 
feels this way, I'm fine with you leaving it as is.

PS25, Line 2186: // Applying a big operation which does not fit into the buffer 
               : // return an error with session running in any supported flush 
> Nope.  This is different: this test verifies that an attempt to apply a _si
Can you help me see the difference? AFAICT the second test in 

1. Starts a new session in MANUAL_FLUSH mode.
2. Customizes the buffer size.
3. Applies a single insert, sized to twice the buffer size.
4. Asserts that this fails with Incomplete.

TestCheckMutationBufferSpaceLimitInEffect does the same thing across all flush 
modes, except that the buffer size is a little different, and that the insert 
isn't twice the buffer size, but only one byte larger.

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 
> About 500 ms.  Is that too much?
Nah, I think that's fine.

Anything on the order of multiple seconds is something we should think about.

Line 2428:   // AUTO_FLUSH_BACKGROUND should be faster than AUTO_FLUSH_SYNC.
> Due to the nature of the things and chosen parameters, the difference is in
Yes, and please try TSAN. It slows things down in unexpected ways (e.g. context 
switches become much more expensive, as does thread startup).

PS25, Line 2574:   ASSERT_TRUE(session->HasPendingOperations());
> Yes, I also have this concern.  The principal solution would be delaying/st
That's fine, just loop it using in TSAN mode, and I'll be 

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