Andrew Wong 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 compatible, given future releases we would just add the SetTransactionOptions() call, rather than change an existing function signature. I would lean towards introducing it later to give ourselves more flexibility though. 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 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 than an argument of this function? Especially knowing that some KuduTransactions may never call Serialize() at all, and thus may not use the options. http://gerrit.cloudera.org:8080/#/c/16948/1/src/kudu/client/client.h@563 PS1, Line 563: SerDesOptions nit: SerializerOptions, same elsewhere http://gerrit.cloudera.org:8080/#/c/16948/1/src/kudu/client/client.h@565 PS1, Line 565: SerDesOptions:: nit: just KuduTransaction::Serialize() now? -- 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: 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 00:35:00 +0000 Gerrit-HasComments: Yes
