Adar Dembo has posted comments on this change. Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients ......................................................................
Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/8026/1//COMMIT_MSG Commit Message: PS1, Line 23: Meanwhile > I read this to mean that this patch is both modifying the master as describ I'll try to clarify while still providing context to understand the motivation for this patch. 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 Done 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 conditio I suppose so, but IsAlterTableDoneRequest behaves this way, as do both IsAlterTableDone and IsCreateTableDone on the C++ side. Moreover, in the standalone case (i.e. if a user just makes those client calls), the table name is all you have anyway. So I'd prefer to keep them consistent. 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? I was copying isAlterTableDone without thinking about it too hard. In order to actually wait for the creation to finish, joining the deferred is insufficient; we need a loop that sleeps and retries too. Line 101: while (totalSleepTime < getDefaultAdminOperationTimeoutMs()) { > This implementation appears to be waiting for the table to be created if it I don't want to change the method name (again, to be consistent with IsAlterTableDone), but I will reword the Javadocs to make this behavior more clear. 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 I think it's OK that it doesn't make much sense since it's debug-level (something we'd normally enable just for isolated testing). I will restructure it (and the version in IsAlterTableDone) though. Line 132: return false; > Shouldn't this throw a timeout exception? That's not the behavior of IsAlterTableDone, and I was trying to be consistent with it. 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 (Not Done Line 954: Insert insert = table.newInsert(); > Shouldn't this work even without waiting for isCreateTableDone? I would ha Yes, we discussed this yesterday. Will make the change. -- 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: 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
