Adar Dembo has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 15: (8 comments) http://gerrit.cloudera.org:8080/#/c/3952/15/src/kudu/client/client.h File src/kudu/client/client.h: Line 1146: Status SetMutationBufferFlushWatermark(double watermark_pct) > Will do. BTW, do we need getters for those parameters? I think getters mi I don't think it's a big deal either way. Sessions are cheap (especially now that they don't have a dedicated thread to flush in the background) and so I tend to think they're short-lived, which means it's easy for the application to remember what value was provided here. http://gerrit.cloudera.org:8080/#/c/3952/15/src/kudu/client/session-internal.cc File src/kudu/client/session-internal.cc: PS15, Line 64: -1, > This is to force getting a new batcher and flushing the prior, if any, even I don't think the existing behavior is necessarily worth preserving; an empty flush doesn't actually accomplish anything. In any case, that wasn't what my comment was about; I was asking about the semantic difference between '-1' and '0' as the watermark value. Line 66: PeriodicFlushTask(Status::OK(), messenger_, session); > OK, I'll change it then. Sorry, I don't understand what you're suggesting. Can you explain it in more detail? Line 109: flow_control_cond_.Broadcast(); > The original idea was to get rid of those limitations like empty buffer whe Nope, simplifying the implementation is the underlying motivation. But I think it's enough; the clients are generally complicated code and aren't as well tested as the rest of Kudu. Line 135: CHECK_GE(timeout_ms, 0); > Yep. What we can do instead is to set it to 0 if the specified parameter w I think that's reasonable provided it's documented. PS15, Line 230: RETURN_NOT_OK(TryApplyWriteOp(write_op, &required_size, : ¤t_flush_mode, ¤t_flush_watermark)); > Yep, having those facts simplifies this. Again, I was under impression the I don't understand why friendship factors into this: both ApplyWriteOp and TryApplyWriteOp are members of KuduSession::Data, so they should be equivalent w.r.t. calling Batcher::SizeInBuffer(). Line 398: data->NextBatcher(session, 0, nullptr); > Nope. It would, however, if the second parameter were -1 (as you noticed t I find the semantics of -1, 0, and >0 to be non-obvious. Perhaps we can convey the desired behavior from caller to PeriodicFlushTask() in another way? Through an additional parameter, an enum, or something like that? http://gerrit.cloudera.org:8080/#/c/3952/15/src/kudu/client/session-internal.h File src/kudu/client/session-internal.h: PS15, Line 87: On successful return, the output flush_mode parameter is set : // to the effective flush mode. > This is because the flush_mode_ can change in the middle and the code which I don't understand how flush_mode_ can change in the middle of ApplyWriteOp if the session may only be used by a single thread. Or is this a holdover from before, when it was OK for multiple threads to write to a session? -- 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: 15 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
