Dan Burkert has posted comments on this change. Change subject: [java client] Support add/remove partition ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/3854/1/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 118: * Multiple range partitions may be added as part of a single alter table transaction by calling > OK, fair. Truth be told, the wait() functionality that's built into the Kud It's a separate call for the Java client, but yes. http://gerrit.cloudera.org:8080/#/c/3854/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: PS1, Line 1380: // This is making this tablet available : // Even if two clients were racing in this method they are putting the same RemoteTablet : // with the same start key in the CSLM in the end > I think you missed this. Done http://gerrit.cloudera.org:8080/#/c/3854/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 349: public Deferred<AlterTableResponse> alterTable(String name, AlterTableOptions ato) { > Why drop the generic type? Done Line 356: // Clear the caches so the new partition is immediately visible. > Nit: update this comment. Done Line 1350: if (existingLocationsCache != null) { > Nit: terminate with period. Done Line 1361: > Nit: "Build the list..." Done Line 1362: // If we already know about this one, just refresh the locations > Nit: "its" Done Line 1365: currentTablet.refreshTabletClients(tabletPb); > Nit: terminate with period. Done Line 1370: // Putting it here first doesn't make it visible because tabletsCache is always looked up > As we discussed, it looks like there's an opportunity to get rid of tablet2 This ended up being sufficiently tricky that I'm punting as part of this change. The issue is that by moving it, we have to build of the remote tablets under the tablet locations cache lock, which involves DNS resolution. http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java: Line 282: String tableName = name.getMethodName(); > Thanks. Could you fix it for the other tests in this file too? Done -- To view, visit http://gerrit.cloudera.org:8080/3854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
