Adar Dembo has posted comments on this change. Change subject: [java client] Support add/remove partition ......................................................................
Patch Set 2: (12 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: * other or any existing range partitions (unless the existing range partitions are dropped as > But it is transactional in the sense that either all operations are applied OK, fair. Truth be told, the wait() functionality that's built into the KuduTableAlterer (C++ client, but I imagine there's an equivalent in Java too) can guarantee that when the "transaction" finishes, all tablets have been altered. 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: if (oldRt != null) { : // someone beat us to it : continue; > Done I think you missed this. 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 alterTable(String name, AlterTableOptions ato) { Why drop the generic type? Line 356: // Clear the caches so the new partition is immediately visible. Nit: update this comment. Line 1350: // already be present Nit: terminate with period. Line 1361: // Build of the list of discovered remote tablet instances. If we have Nit: "Build the list..." Line 1362: // already discovered the tablet, it's locations are refreshed. Nit: "its" Line 1365: // Early creating the tablet so that it parses out the pb Nit: terminate with period. Line 1370: RemoteTablet currentTablet = tablet2client.get(tabletId); As we discussed, it looks like there's an opportunity to get rid of tablet2client entirely, provided 1) we replace this lookup with a lookup into the main table locations cache, 2) adjust the locking so that there's no chance of a race between this lookup and the innards of cacheTabletLocations(). http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java File java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java: Line 170: if (floorEntry != null && > Line 167 is limiting the removed set to the suffix after the lower bound, w I see. I missed that this line acts on overlappingEntries, not entries. Line 200: public static class Entry { > If you insist I can change it, but I think DeadlineTrackers are overkill fo I don't feel strongly, no. 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() + System.currentTimeMillis(); > Done Thanks. Could you fix it for the other tests in this file too? -- 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: 2 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
