Dan Burkert has posted comments on this change. Change subject: [java client] Support add/remove partition ......................................................................
Patch Set 2: (17 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 sha Correct; it's only an issue if we take a Guava type as an argument, or return a Guava type. Line 37: /** > Hmm, can we make this private? Done Line 118: * other or any existing range partitions (unless the existing range partitions are dropped as > Nit: I get that this is copied from the C++ client, but we should probably But it is transactional in the sense that either all operations are applied, or they are all rejected atomically. We sometimes overload operation to mean a specific step, and sometimes the whole set of steps. 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 697: // called via a callback that won't care about the returned Deferred. : request.errback(e); > There's no danger of a code path that would "double count" the error by vir I'm not really sure what it means to count an error? But my understanding is that either callers are using the deferred, or they are using the errback chain of the request. Why we use one in some cases and the other in others is a mystery to me. PS1, Line 1343: List<Master.TabletLocationsPB> locations, : long ttl) throws NonRecoverableException > This comment needs to be updated. Done Line 1366: RemoteTablet rt = createTabletFromPb(tableId, tabletPb); > Why this change? Doesn't it contradict this block's comment? Yah, the way this works has changed. We need to send the existing tablets to the table locations cache in order to update the TTL, and use it to discover non-covered ranges. I've updated the comment. PS1, Line 1380: if (oldRt != null) { : // someone beat us to it : continue; > Needs to be updated. To be honest, all the comments in this method could st Done PS1, Line 1395: locationsCache.cacheTabletLocations(tablets, requestPartitionKey, ttl); : } : : RemoteTablet createTabletFromPb(String tableId, Master.TabletLocationsPB tabletPb) { : Partition partition = ProtobufHelper.pbToPartition(tabletPb.getPartition()); : S > Update this. Done 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: * > As written, each new entry is likely to have a slightly different deadline, Done PS1, Line 99: List<Entry> newEntries = new ArrayList<>(); : > Nit: indentation. Done Line 170: if (floorEntry != null && > If we're executing this line, why bother executing L167-168? Line 167 is limiting the removed set to the suffix after the lower bound, while this line is limiting the removed set the the prefix before the upper bound. Together, they limit the remove set between the two bounds. Line 200: public static class Entry { > Maybe we can track these via DeadlineTrackers? If you insist I can change it, but I think DeadlineTrackers are overkill for this. This deadline doesn't need to be resettable, and I have a hard time figuring out the DeadlineTracker API to begin with. Why the heck does it need a Stopwatch? Why is it resettable? Line 246: return tablet == null ? lowerBoundPartitionKey : tablet.getPartition().getPartitionKeyStart(); > Should we prevent this value from becoming negative (i.e. return zero if th I've changed this to be a private method, since it's only used for printing. I think it may be useful in some circumstances to see how expired an entry is. 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: // Licensed to the Apache Software Foundation (ASF) under one > Missing copyright statement. Done PS1, Line 32: : > That's not true if I pass something into 'bounds', right? Done 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(); > Note that we need to add system time to these table names, because if the t Done 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 st Maybe, but I'd like these two error cases to be somewhat consistent. I could change the Add case to AlreadyPresent and the Drop to NotFound, but this might be an issue for applications which assume AlreadyPresent means the table rename failed? -- 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
