Dinesh Bhat has posted comments on this change. Change subject: KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode ......................................................................
Patch Set 19: (17 comments) Hi Alexey, went through this patch more from knowledge point of view, so feel free to ignore any review comments you find not useful. Also I haven't completely digested all the routines in sesion-internal.cc, so I may as well take a second pass tomorrow later if time permits. http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: PS18, Line 522: we Does it make sense to add a coment that callers are expected to serialize during Add ? http://gerrit.cloudera.org:8080/#/c/3952/16/src/kudu/client/batcher.h File src/kudu/client/batcher.h: Line 168: typo: the the PS16, Line 169: ck s/is/are/ http://gerrit.cloudera.org:8080/#/c/3952/17/src/kudu/client/batcher.h File src/kudu/client/batcher.h: PS17, Line 122: int64_t Naive doubt here: If Load() is returning a non-atomic value we seem to be accessing the atomic value of buffer_bytes_used_ in write path vs non-atomic in read path ? http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 692: : data_(new KuduSession::Data(client, client->data_->messenger_)) { I haven't looked at how data_ being set, just wondering if data_ could be NULL since it seems to be a regular ptr. PS18, Line 736: int uint ? PS18, Line 740: int uint ? http://gerrit.cloudera.org:8080/#/c/3952/16/src/kudu/client/client.h File src/kudu/client/client.h: Line 1017: /// Modes of flush operations. Is there a default mode of op here ? if so, can we mention it in comment ? Line 1038: /// batch is sent and the reclamed space is available for new operations. typo: reclamed Line 1040: /// @todo Provide an API for the user to specify a callback to do their own If this is not specific to this mode alone, we could move this todo in the beginning perhaps ? Line 1044: /// (probably the messenger IO threads?). I guess this todo is already addressed ? http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/session-internal.cc File src/kudu/client/session-internal.cc: PS18, Line 448: Nice !! Liked these ascii diagrams. http://gerrit.cloudera.org:8080/#/c/3952/19/src/kudu/client/session-internal.cc File src/kudu/client/session-internal.cc: PS19, Line 233: weak_session any significance of usage of 'weak' session ? :) PS19, Line 263: ( Isn't this redundant ? We could as well place NextBatcher above ? do { NextBatcher(); CanApplyWriteOp(&need_pre_flush); } while (need_pre_flush) PS19, Line 270: lock_ I thought whole routine is serialized, so why another spinlock here ? http://gerrit.cloudera.org:8080/#/c/3952/18/src/kudu/client/session-internal.h File src/kudu/client/session-internal.h: PS18, Line 126: to s/to/for ? PS18, Line 165: cond_mutex_ Trivial nit: I suggest to rename this to flow_control_cond_mutex_ which could be self explanatory(and remove al the comment associated). Just a suggestion, feel free to ignore this. -- 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: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
