Adar Dembo has posted comments on this change.
Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
Patch Set 23:
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.
PS23, Line 42: It's
PS23, Line 43: implemented
Nit: don't need this word.
PS23, Line 128: // The self-rescheduling task to flush write operations which
: // 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
PS23, Line 172: requries
To view, visit http://gerrit.cloudera.org:8080/3952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>