Adar Dembo has posted comments on this change. Change subject: [java client] Support add/remove partition ......................................................................
Patch Set 1: (18 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 19: import com.google.common.base.Preconditions; This doesn't make Guava part of the API itself, right? This will be our shaded Guava? Line 37: AlterTableRequestPB.Builder pb = AlterTableRequestPB.newBuilder(); Hmm, can we make this private? Line 118: * Multiple range partitions may be added as part of a single alter table transaction by calling Nit: I get that this is copied from the C++ client, but we should probably replace "transaction" with "operation" (below too). We don't want users to think that once the AlterTable is done, a "transaction" has been committed across the cluster. 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 360: tableLocations.clear(); : tablet2client.clear(); What's the intuition behind clearing these two but not client2tablet? PS1, Line 697: // Sending both as an errback and returning fromError because sendRpcToTablet might be : // called via a callback that won't care about the returned Deferred. There's no danger of a code path that would "double count" the error by virtue of installing an errback() AND considering the return value, is there? PS1, Line 1343: // Doing a get first instead of putIfAbsent to avoid creating unnecessary CSLMs because in : // the most common case the table should already be present This comment needs to be updated. Line 1366: tablets.add(currentTablet); Why this change? Doesn't it contradict this block's comment? 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 Needs to be updated. To be honest, all the comments in this method could stand to be refined a bit. PS1, Line 1395: /** : * Gives the tablet's ID for the table ID and partition key. : * In the future there will be multiple tablets and this method will find the right one. : * @param tableId table to find the tablet for : * @return a tablet ID as a slice or null if not found : */ Update this. 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 77: long ttl) { As written, each new entry is likely to have a slightly different deadline, which is a bit odd: they were created en masse here, so shouldn't they expire en masse too? To fix, you could make just one call to System.nanoTime(), compute the deadline, and passing it into each new entry. PS1, Line 99: AsyncKuduClient.EMPTY_ARRAY, : ttl)); Nit: indentation. Line 170: overlappingEntries = overlappingEntries.headMap(discoveredUpperBound, false); If we're executing this line, why bother executing L167-168? Line 200: private final long deadline; Maybe we can track these via DeadlineTrackers? Line 246: return deadline - System.nanoTime(); Should we prevent this value from becoming negative (i.e. return zero if the deadline expires)? http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java: Line 1: package org.apache.kudu.client; Missing copyright statement. PS1, Line 32: and range partitioned : * on c0 with no range bounds. That's not true if I pass something into 'bounds', right? 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(); Note that we need to add system time to these table names, because if the test fails and is retried, I don't believe the cluster state is cleaned out in between, so there'd be a table name collision. http://gerrit.cloudera.org:8080/#/c/3854/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 1347: return Status::InvalidArgument("No tablet found for drop partition step", Isn't "not found" a better match for "no tablet found for drop partition step" though? -- 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: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
