Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16948 )
Change subject: WIP [client] updated transaction API ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/16948/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16948/1//COMMIT_MSG@23 PS1, Line 23: As for the compatibility perspective, it can be introduced later on : without breaking API/ABI compatibility. > Right, from an ABI/API perspective, adding this later would be backwards co Yep, that makes sense to me, thanks! I just wanted to explore this option as the result of our recent discussion. http://gerrit.cloudera.org:8080/#/c/16948/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/16948/1/src/kudu/client/client.h@460 PS1, Line 460: Serializer > nit: there's no "serializer" class so maybe call this SerializeOptions or S Yep, make sense. It seems this name is a pure consequence of having too many iterations over this simple API patch :) I changed this to SerializationOptions. http://gerrit.cloudera.org:8080/#/c/16948/1/src/kudu/client/client.h@557 PS1, Line 557: Serialize(s > I'm curious why the preference to have SerializerOptions as a member rather I wanted to be able to call this method as just Serialize(&txn_token), so then the serialization options would become the second parameter with 'the parameter by default' signature. However, that would be a bit awkward since it would contradict our coding style: input parameters come first, and output parameters come last. I think the signature like Serialize(options, &txn_token) is fine, and we can go forward with that. A agree that it looks better from the perspective of: * keeping less state in a transaction handle * having less methods in the interface of KuduTransaction http://gerrit.cloudera.org:8080/#/c/16948/1/src/kudu/client/client.h@563 PS1, Line 563: SerDesOptions > nit: SerializerOptions, same elsewhere Done http://gerrit.cloudera.org:8080/#/c/16948/1/src/kudu/client/client.h@565 PS1, Line 565: SerDesOptions:: > nit: just KuduTransaction::Serialize() now? Done -- To view, visit http://gerrit.cloudera.org:8080/16948 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id646c41d60f385f079ebfaec2daf7c2a108306b2 Gerrit-Change-Number: 16948 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Jan 2021 03:13:12 +0000 Gerrit-HasComments: Yes
