Dan Burkert has posted comments on this change. Change subject: KUDU-1807 (part 1): ban GetTableSchema for table createdness in clients ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/8026/1/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 524: * Gets a table's schema Missing period http://gerrit.cloudera.org:8080/#/c/8026/1/java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java: Line 36: IsCreateTableDoneRequest(KuduTable table, String name) { Doesn't identifying the table by name open the client up to a race condition wherein another table is swapped in with the name? http://gerrit.cloudera.org:8080/#/c/8026/1/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: Line 99: public boolean isCreateTableDone(String name) throws KuduException { Why is this not a joinAndHandleException wrapper around an async variant? Line 101: while (totalSleepTime < getDefaultAdminOperationTimeoutMs()) { This implementation appears to be waiting for the table to be created if it's not already. I'd suggest not doing that, or otherwise changing the name of the method. Line 121: LOG.debug("Create not done, sleep " + (AsyncKuduClient.SLEEP_TIME - elapsed) + This log message doesn't make a lot of sense in isolation, and restructure it to be: LOG.debug("Create not done, sleep {} and slept {}", AsyncKuduClient.SLEEP_TIME - elapsed, totalSleepTime); Line 132: return false; Shouldn't this throw a timeout exception? http://gerrit.cloudera.org:8080/#/c/8026/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: Line 940: if (e.getMessage().contains("does not exist")) { I think this could be more precise by asserting on the returned Status (NotFound). Line 954: Insert insert = table.newInsert(); Shouldn't this work even without waiting for isCreateTableDone? I would have thought the tablet location lookups would keep retrying until the tablets were created. -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
