Alexey Serbin 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
> Just to be clear, I meant "coarser" where I wrote "higher" granularity. Tha
I understand the concern.  What if we put 1 ms? :)

Frankly, this kind of check is not really reliable anyway.  It can spot an 
issue if operations are slowly pushed to tablet servers, but otherwise this 
check might not catch issues.

I just could not find anything else which would not be invasive.  Keeping max 
value in the session itself does not seem to be a good solution.  The other 
solution involves building mock Batcher or Messenger which seems to be an 

PS25, Line 2186: // Applying a big operation which does not fit into the buffer 
               : // return an error with session running in any supported flush 
> Can you help me see the difference? AFAICT the second test in TestApplyTooM
Yes, after looking at TestApplyTooMuchWithoutFlushing once more I understand 
you are right.  For some reason I was thinking that the second part of the 
TestApplyTooMuchWithoutFlushing was about big operations where each fits the 
buffer, but eventually they overflow it.  My bad. 

OK, I'll remove that second part of the TestApplyTooMuchWithoutFlushing test.  
Because this test is a superset of the former: it verifies the behavior is 
correct for all flush modes.

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 
> Nah, I think that's fine.
OK, I see.  Thanks for the guideline.

Line 2428:   // AUTO_FLUSH_BACKGROUND should be faster than AUTO_FLUSH_SYNC.
> Yes, and please try TSAN. It slows things down in unexpected ways (e.g. con
Yep, did that as well.  However, I'm not sure I got 1000 cycles -- that was too 
slow.  Will run tonight.

PS25, Line 2574:   ASSERT_TRUE(session->HasPendingOperations());
> That's fine, just loop it using in TSAN mode, and I'll be sati
ok, will do.

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