Dan Burkert has posted comments on this change.

Change subject: [java client] Support add/remove partition

Patch Set 2:



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?

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.

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.

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

PS1, Line 1395:     locationsCache.cacheTabletLocations(tablets, 
requestPartitionKey, ttl);
              :   }
              :   RemoteTablet createTabletFromPb(String tableId, 
Master.TabletLocationsPB tabletPb) {
              :     Partition partition = 
              :     S
> Update this.


Line 77:    *
> As written, each new entry is likely to have a slightly different deadline,

PS1, Line 99:     List<Entry> newEntries = new ArrayList<>();
> Nit: indentation.

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 : 
> 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.

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.

PS1, Line 32: 
> That's not true if I pass something into 'bounds', right?

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 t

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 

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

Reply via email to