Adar Dembo has posted comments on this change.

Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode

Patch Set 23:

File src/kudu/client/

Line 63:   TimeBasedFlushInit(session);
> How about checking if we are in AUTO_FLUSH_BACKGROUND first?  I know this i
The funny thing is that, by definition, we cannot be in AUTO_FLUSH_BACKGROUND, 
because the default flush mode is AUTO_FLUSH_SYNC. But, that can change in the 

In any case, I think TimeBasedFlushInit() is a no-op in AUTO_FLUSH_BACKGROUND 

PS23, Line 105:    std::lock_guard<simple_spinlock> l(lock_);
Since this isn't acquired on any hot path, what do you think of removing it in 
favor of batch_ctl_mutex_? I think it's a little easier to reason about the 
flow if we can minimize the number of locks.

If you do end up doing this, probably best to rename batch_ctl_mutex_ to lock_ 
(and maybe rename batch_ctl_cond_ too).

Line 181:   FlushCurrentBatcher(1, cb);
Nit: add a comment explaining the significance of 1 (i.e. why not 0?)

PS23, Line 188: nullptr
I spent a lot of time wondering if the loss of the synchronizer and 
RETURN_NOT_OK(s.Wait()) meant we were dropping errors on the floor. Meaning, is 
there any case where the batcher invokes the flush callback with !Status::OK() 
and does NOT write errors to the error collector?

I don't think there are, but the flow is tricky enough that I thought I'd ask 
you the same question.

BTW, it would be worth documenting in client.h the difference in behavior 
between Flush() and FlushAsync() w.r.t. waiting for outstanding batches to 
flush. Or do you think it's implicitly understood?

PS23, Line 296: The flush_mode_ is accessed
              :   // from the background time-based flush task, but it accesses 
              :   // only for reading.
TSAN may complain about this. ISTR it doesn't like accesses to the same members 
with a mixed set of locks (i.e. one lock held in one access, and no locks in 

Line 354:         batch_ctl_cond_.Wait();
This isn't going to work for ApplyAsync(). Now, we haven't actually implemented 
ApplyAsync() yet, so there's no reason to do anything. But we should start 
thinking about how ApplyWriteOp() would work in an ApplyAsync() world, 
hopefully without the creation of two separate apply code paths.
File src/kudu/client/session-internal.h:

PS23, Line 42: It's
Nit: Its

PS23, Line 43: implemented
Nit: don't need this word.

PS23, Line 128:   // The self-rescheduling task to flush write operations which 
have been
              :   // accumulating for too long (controlled by flush_interval_).
              :   // This does real work only in case of AUTO_FLUSH_BACKGROUND 
Should document what 'do_startup_check' means.

PS23, Line 172: which
Nit: "whose"

PS23, Line 172: requries
Nit: "requires"

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8
Gerrit-PatchSet: 23
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