Dan Burkert has posted comments on this change.

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


Patch Set 1:

(10 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:    * Multiple range partitions may be added as part of a single alter 
table transaction by calling
> OK, fair. Truth be told, the wait() functionality that's built into the Kud
It's a separate call for the Java client, but yes.


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:       // 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
> I think you missed this.
Done


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<AlterTableResponse> alterTable(String name, 
AlterTableOptions ato) {
> Why drop the generic type?
Done


Line 356:       // Clear the caches so the new partition is immediately visible.
> Nit: update this comment.
Done


Line 1350:       if (existingLocationsCache != null) {
> Nit: terminate with period.
Done


Line 1361: 
> Nit: "Build the list..."
Done


Line 1362:       // If we already know about this one, just refresh the 
locations
> Nit: "its"
Done


Line 1365:         currentTablet.refreshTabletClients(tabletPb);
> Nit: terminate with period.
Done


Line 1370:       // Putting it here first doesn't make it visible because 
tabletsCache is always looked up
> As we discussed, it looks like there's an opportunity to get rid of tablet2
This ended up being sufficiently tricky that I'm punting as part of this 
change.  The issue is that by moving it, we have to build of the remote tablets 
under the tablet locations cache lock, which involves DNS resolution.


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();
> Thanks. Could you fix it for the other tests in this file too?
Done


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

Reply via email to