Adar Dembo has posted comments on this change. Change subject: Add table id to AlterTableResponsePB ......................................................................
Patch Set 2: (4 comments) Will you be making a similar change to the C++ client? http://gerrit.cloudera.org:8080/#/c/3859/2/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java: Line 26: String tableId; private? Line 37: * @return the ID of the altered table, or null if the master version is too old (< 0.10) I think we should avoid embedding version numbers directly in the code like this. "Returns the table ID or null if the master was too old" is sufficient. It's kind of a capabilities vs. versions thing; if it's desirable for users to know whether a piece of functionality exists, we should expose it via a capability or feature flag. http://gerrit.cloudera.org:8080/#/c/3859/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: Line 361: // version to return the table ID (>= 0.10), we can selectively clear Nit: version string here too. Line 365: if (resp instanceof AlterTableResponse) { The type erasure here is annoying. The only way to avoid it would be to duplicate the callback for addErrback(), right? Maybe we can do that without duplicating all of the code that the two invoke? -- To view, visit http://gerrit.cloudera.org:8080/3859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
