You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/08/05 23:47:53 UTC
[kudu-CR] [java client] Support add/remove partition
Hello Adar Dembo, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/3854
to review the following change.
Change subject: [java client] Support add/remove partition
......................................................................
[java client] Support add/remove partition
This also sneaks a fix into catalog manager to change the status type when
rejecting invalid alter table partitioning operations.
Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
D java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
A java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
M src/kudu/master/catalog_manager.cc
13 files changed, 830 insertions(+), 276 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/3854/1
--
To view, visit http://gerrit.cloudera.org:8080/3854
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [java client] Support add/remove partition
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3854
to look at the new patch set (#2).
Change subject: [java client] Support add/remove partition
......................................................................
[java client] Support add/remove partition
This also sneaks a fix into catalog manager to change the status type when
rejecting invalid alter table partitioning operations.
Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
D java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
A java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
M src/kudu/master/catalog_manager.cc
13 files changed, 877 insertions(+), 289 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/3854/2
--
To view, visit http://gerrit.cloudera.org:8080/3854
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [java client] Support add/remove partition
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [java client] Support add/remove partition
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] Support add/remove partition
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2726/
--
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 <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [java client] Support add/remove partition
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [java client] Support add/remove partition
......................................................................
Patch Set 1:
(1 comment)
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?
This only needs to clear the locations cache. I changed it to just that.
--
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 <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [java client] Support add/remove partition
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Support add/remove partition
......................................................................
Patch Set 3: Code-Review+2
--
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [java client] Support add/remove partition
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [java client] Support add/remove partition
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Support add/remove partition
......................................................................
Patch Set 2:
(12 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: * other or any existing range partitions (unless the existing range partitions are dropped as
> But it is transactional in the sense that either all operations are applied
OK, fair. Truth be told, the wait() functionality that's built into the KuduTableAlterer (C++ client, but I imagine there's an equivalent in Java too) can guarantee that when the "transaction" finishes, all tablets have been altered.
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: if (oldRt != null) {
: // someone beat us to it
: continue;
> Done
I think you missed this.
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 alterTable(String name, AlterTableOptions ato) {
Why drop the generic type?
Line 356: // Clear the caches so the new partition is immediately visible.
Nit: update this comment.
Line 1350: // already be present
Nit: terminate with period.
Line 1361: // Build of the list of discovered remote tablet instances. If we have
Nit: "Build the list..."
Line 1362: // already discovered the tablet, it's locations are refreshed.
Nit: "its"
Line 1365: // Early creating the tablet so that it parses out the pb
Nit: terminate with period.
Line 1370: RemoteTablet currentTablet = tablet2client.get(tabletId);
As we discussed, it looks like there's an opportunity to get rid of tablet2client entirely, provided 1) we replace this lookup with a lookup into the main table locations cache, 2) adjust the locking so that there's no chance of a race between this lookup and the innards of cacheTabletLocations().
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 170: if (floorEntry != null &&
> Line 167 is limiting the removed set to the suffix after the lower bound, w
I see. I missed that this line acts on overlappingEntries, not entries.
Line 200: public static class Entry {
> If you insist I can change it, but I think DeadlineTrackers are overkill fo
I don't feel strongly, no.
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();
> Done
Thanks. Could you fix it for the other tests in this file too?
--
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 <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [java client] Support add/remove partition
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3854
to look at the new patch set (#3).
Change subject: [java client] Support add/remove partition
......................................................................
[java client] Support add/remove partition
This also sneaks a fix into catalog manager to change the status type when
rejecting invalid alter table partitioning operations.
Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
D java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
A java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
M src/kudu/master/catalog_manager.cc
13 files changed, 879 insertions(+), 294 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/3854/3
--
To view, visit http://gerrit.cloudera.org:8080/3854
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [java client] Support add/remove partition
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: [java client] Support add/remove partition
......................................................................
[java client] Support add/remove partition
This also sneaks a fix into catalog manager to change the status type when
rejecting invalid alter table partitioning operations.
Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
Reviewed-on: http://gerrit.cloudera.org:8080/3854
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
D java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
A java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
M src/kudu/master/catalog_manager.cc
13 files changed, 879 insertions(+), 294 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3854
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [java client] Support add/remove partition
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] Support add/remove partition
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/2773/
--
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [java client] Support add/remove partition
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [java client] Support add/remove partition
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/2766/
--
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 <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [java client] Support add/remove partition
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes