Adar Dembo has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 23: (11 comments) http://gerrit.cloudera.org:8080/#/c/3952/23/src/kudu/client/session-internal.cc File src/kudu/client/session-internal.cc: 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 future. In any case, I think TimeBasedFlushInit() is a no-op in AUTO_FLUSH_BACKGROUND mode. 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 it : // 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 another). 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. http://gerrit.cloudera.org:8080/#/c/3952/23/src/kudu/client/session-internal.h 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 mode. Should document what 'do_startup_check' means. PS23, Line 172: which Nit: "whose" PS23, Line 172: requries Nit: "requires" -- To view, visit http://gerrit.cloudera.org:8080/3952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34905c30b3aad96f53cf7a1822b1cde6d25f33a8 Gerrit-PatchSet: 23 Gerrit-Project: kudu Gerrit-Branch: master 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> Gerrit-HasComments: Yes
