David Ribeiro Alves has posted comments on this change. Change subject: Expose row format flags in KuduScanner ......................................................................
Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/6624/6/src/kudu/client/client.cc File src/kudu/client/client.cc: PS6, Line 1247: Raw mode > "Row format flags" Done PS6, Line 1254: raw mode > "row format" Done http://gerrit.cloudera.org:8080/#/c/6624/6/src/kudu/client/client.h File src/kudu/client/client.h: PS6, Line 2053: 0; > Should this be NO_FLAGS? Done http://gerrit.cloudera.org:8080/#/c/6624/6/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: Line 545: if (row_format_flags_ != KuduScanner::NO_FLAGS) { > Why is this check a LOG(FATAL) one here but a DCHECK_EQ() one in KuduScanBa This one is not on a hot path while the other one is. I'd prefer to have it fatal there too, but I explicitly called out that the client might crash _or_ return wrong results. PS6, Line 546: Format modifier > Row format Done http://gerrit.cloudera.org:8080/#/c/6624/6/src/kudu/client/scanner-internal.h File src/kudu/client/scanner-internal.h: PS6, Line 272: modifiers > flags Done Line 304: int64_t row_format_flags_; > Nit: since every other data member here is documented, could you document t Done http://gerrit.cloudera.org:8080/#/c/6624/6/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: PS6, Line 274: IN > Nit: in (or did you capitalize this for a reason?) hum, no, weird. Done PS6, Line 275: 0 > Shouldn't this be RowFormatFlags::NO_FLAGS? I had tried that. protoc doesn't let me assign an enum to an int64. Did point that this value corresponds to NO_FLAGS thohd -- To view, visit http://gerrit.cloudera.org:8080/6624 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
