Adar Dembo has posted comments on this change. Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients ......................................................................
Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java File java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java: Line 361: * considered to be successful. > What happens if you elect to not wait? What's the expectation here? Right n If you don't wait, you may operate on the table before the alter is finished. Depending on what you're altering, the operation may or may not be impacted: - A renamed table will have no discernible effect. - Inserts to an added range partition will retry until that partition is created (so no effect). - A changed schema (i.e. add/modify/drop column) won't be propagated to clients in openTable() until the alter is done. Not waiting is useful if you want to alter a lot of tables as quickly as possible (i.e. as an admin tool). I also think it's good to have just for feature parity with the C++ client. FWIW, the C++ client documents the 'wait' functionality of creating/altering a table rather vaguely since the specifics are definitely "implementation detail" level. 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' f I didn't know we used those annotations, but I see a couple other places in the Java client where we do. Sure, why not. http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java File java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java: Line 191: * Whether to wait for all tablets to be created before the table creation is > What happens if you set this to false? Is your client going to blow up when It's hard to doc this completely without mentioning some implementation details. The C++ client is even more vague, saying that wait(true) means to wait "until the table is fully created" before returning. I think it's reasonable to mention the existence of "tablets", though. That's what I documented in AlterTableOptions. As for your other questions: 1. The client won't blow up if you use the table, though certain operations may retry under the hood. 2. I think this is useful for batch creation of many tables without explicitly waiting for any of them, and for feature parity with the C++ client. As an aside, I do appreciate your questions about how to communicate this stuff to users, and if you have concrete doc suggestions I'll definitely consider them. PS3, Line 199: setWait > I disagree. It's just called 'wait' in the C++ API. I agree with Dan; I think being more vague is better in this case. If we're too specific, we open the door for confusion down the road if/when we want to tack on additional sub-operations to CreateTable and "tablet assignment" or whatever we've written here no longer captures it all. http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java: Line 58: Preconditions.checkNotNull(name); > This precondition is already checked in the constructor. Yes, but this is now moot: I reworked this stuff to take a TableIdentifierPB.Builder in the constructor (to better encapsulate the "name vs. id" dichotomy). http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java: Line 57: Preconditions.checkNotNull(name); > Likewise. Moot for the same reason. http://gerrit.cloudera.org:8080/#/c/8026/3/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 78: Preconditions.checkNotNull(name); > Likewise Moot. 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 so Agreed, but that ship has sailed on isAlterTableDone, and I'd rather the two be consistent with one another than one have a more correct name. I'm hoping the Javadoc will help here. Line 99: while (timeSleptMillis < getDefaultAdminOperationTimeoutMs()) { > do you think it's possible to factor out this "retry running an async metho I did you one better: I refactored the asynchronous IsFooTableDone RPC loop so that it's usable from here, and dropped these handwritten synchronous loops entirely. PS3, Line 166: isAlterTableDone > same, although I guess this one was already implemented, so it's hard to re Right, and because this one has a crappy name, I've opted to give the new one a crappy name too, for Consistency (tm). http://gerrit.cloudera.org:8080/#/c/8026/3/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 947: Future<?> fut = exec.submit(new Runnable() { > why not just use deferreds / the async client? Sure, why not. -- 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
