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 <d...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to