Dan Burkert has posted comments on this change. Change subject: KUDU-1312: scan token protobuf message format ......................................................................
Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/2622/6/src/kudu/client/client.proto File src/kudu/client/client.proto: Line 26: message ScanTokenPB { > Is it at all possible to reuse NewScanRequestPB? It's a shame that new fiel It may be, but some of the fields don't match exactly. Todd was in favor of a separate type when I asked him about reusing NewScanRequestPB. Line 40: optional string table_name = 2; > Why optional? Isn't the name of the table a requirement? If not, could you My understanding is that a protobuf best practice never use required fields due to backwards compatibility concerns. For example, Protobuf 3 has not notion of required/optional (everything is implicitly optional). Line 58: // Encoded partition key to begin scanning at (inclusive). : optional bytes lower_bound_partition_key = 7; : : // Encoded partition key to stop scanning at (exclusive). : optional bytes upper_bound_partition_key = 8; > These aren't in NewScanRequestPB. Why do we need them here and not there? They aren't needed in NewScanRequestPB because that message is meant for sending from clients to tablets (and carries an associated tablet_id), and so implicitly carries the partition key range of the tablet. They are needed here to restrict the scanner to a subset of the tablets in the table. An alternative could have been using a tablet_id field instead of the partition key range, but that would break if we ever implement range splitting and the tokens were created before a split and executed after the split. Line 87: optional bool fault_tolerant = 14 [default = false]; > Shouldn't this use OrderMode? I'm not sure. We've deprecated OrderMode from the client API in preference to setting fault tolerance, but didn't change the NewScanRequestPB. Given that this is a client-only API I think it makes sense to have it be fault tolerance. Line 88: } > Do we need last_primary_key too? no, this is only used for creating new scanners. If the primary key range needs to be restricted it can be done via lower/upper bound primary key. -- To view, visit http://gerrit.cloudera.org:8080/2622 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09983d71c81a383cf4e0e24a49367c64960bbd4d Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
