Todd Lipcon has posted comments on this change. Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/8026/3/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 595: String tableId, you think its' worth adding @Nullable to this and @Nonnull to 'tableName' for better documentation and static-analysis value? If not, no worries. http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java: PS3, Line 97: isCreateTableDone this seems misnamed -- shouldn't it be 'waitForCreateTableCompletion' or something, considering it ostensibly blocks for up to the provided timeout? Line 99: while (timeSleptMillis < getDefaultAdminOperationTimeoutMs()) { do you think it's possible to factor out this "retry running an async method up to a provided timeout" pattern that we see here and in isAlterTableDone? Seems like the methods are the same modulo which function they call into, so maybe could be something like: private <T> callAsyncWithSleepsAndRetries(Callable<Deferred<T>> func, String taskNameForLogs) { ... } PS3, Line 166: isAlterTableDone same, although I guess this one was already implemented, so it's hard to rename it without a deprecation period. -- To view, visit http://gerrit.cloudera.org:8080/8026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
