You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/09/10 00:21:33 UTC

[kudu-CR] KUDU-1807 (part 1): ban GetTableSchema for table createdness in clients

Hello Dan Burkert, Jean-Daniel Cryans, Todd Lipcon,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/8026

to review the following change.

Change subject: KUDU-1807 (part 1): ban GetTableSchema for table createdness in clients
......................................................................

KUDU-1807 (part 1): ban GetTableSchema for table createdness in clients

Improving IsCreateInProgress performance in the master is very challenging
due to the myriad of locks in play. A simpler alternative would be to get it
off any critical paths. It's currently called by the IsCreateTableDone and
GetTableSchema RPCs. The former makes sense but the latter does not: if we
need to know whether a table has been created, well, that's what the
IsCreateTableDone RPC is for.

So, does anyone use the "is table done creating" aspect of GetTableSchema?
The C++ client doesn't, but the Java client does. As luck would have it, the
Java client is already written to fall back to IsCreateTableDone if
GetTableSchema says the table is still being created, which means we can
safely modify the master to always return false in this field without
breaking old Java clients.

Meanwhile, this patch modifies new Java clients to no longer use that field.
After the CreateTable RPC returns, the client will enter an IsCreateTableDone
loop, and when that's done, it'll issue a GetTableSchema to fetch the schema
and build a KuduTable.

There are a couple other changes worth noting:
- openTable no longer waits for table creation before returning. Doing that
  now would require at least one IsCreateTableDone RPC, which unnecessarily
  slows down table opening in the common case. The C++ client doesn't do
  this, so I don't see why the Java client should.
- I removed the 'tablesNotServed' logic which, AFAICT, had no useful effect
  since clients are supposed to wait for tables to be fully created before
  writing/scanning anyway.
- I removed the master permit acquisition from the "is create table done"
  loop, since these loops should no longer produce a thundering herds (as I
  suppose they could have due to 'tablesNotServed').
- Even though createTable waits for table creation to finish, I added an
  isCreateTableDone method to help synchronized the use case where multiple
  clients or threads all create and open a table concurrently (e.g. YCSB).

Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeadlineTracker.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java
A java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestDeadlineTracker.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
12 files changed, 340 insertions(+), 241 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/8026/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8026/2//COMMIT_MSG
Commit Message:

PS2, Line 20: with TABLET_NOT_RUNNING
            :   master errors
Mind pointing me where that is?


http://gerrit.cloudera.org:8080/#/c/8026/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 641:           // TODO(adar): how can we do getTableSchemaRpc.setParent(rpc)?
Have a getTableSchema that takes a parent rpc?


-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8026/3/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 361:    * considered to be successful.
> That's why I don't like this. We don't want to document this because it's "
Let me try to walk this fine line...


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
File java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java:

Line 191:    * Whether to wait for all tablets to be created before the table creation is
> I think we can't have our cake and eat it too, if we want to expose a publi
OK, I'll try to doc the choice between waiting and not waiting more clearly, without getting into implementation details.


PS3, Line 199: setWait
> Will the new sub-operations also require new setters? We're basically alrea
Sorry for not being clear. What I meant is that this wait(), by virtue of being vague, would include waiting on new sub-operations too. It'd be an all-encompassing wait(), which I think is desirable (I don't think waiting for some operations but not others is particularly useful).


-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................

Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8026/3/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 361: * considered to be successful.
> If you don't wait, you may operate on the table before the alter is finishe
That's why I don't like this. We don't want to document this because it's "implementation details" but then make it a public API.

http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
File java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java:

Line 191: * Whether to wait for all tablets to be created before the table creation is
> It's hard to doc this completely without mentioning some implementation det
I think we can't have our cake and eat it too, if we want to expose a public API that's about an implementation detail then we either document what it's doing or we don't add an obscure API. I do think a high level sketch of what this method does is required. Else what is it good for?

PS3, Line 199: setWait
> I agree with Dan; I think being more vague is better in this case. If we're
Will the new sub-operations also require new setters? We're basically already going down that route. I think there's room for more documentation.

--
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8026/3/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 361:    * considered to be successful.
What happens if you elect to not wait? What's the expectation here? Right now this feels like a "Kudu dev-only" API.


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
File java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java:

Line 191:    * Whether to wait for all tablets to be created before the table creation is
What happens if you set this to false? Is your client going to blow up when you try to use the KuduTable?

Also, isn't waiting here kind of an implementation detail? It's not known to the user that there happens to be a call to create a table, which returns after the master accepts it but still hasn't assigned the tablets. Why are we calling this out as an API?


PS3, Line 199: setWait
I'd be more specific about what it waits for, setWaitForTableAssigned or something?


-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8026/1/java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java
File java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java:

Line 36:   IsCreateTableDoneRequest(KuduTable table, String name) {
> I suppose so, but IsAlterTableDoneRequest behaves this way, as do both IsAl
I believe it's wrong to use the table name in any context where the KuduTable object is available, since it should have an attached Kudu table ID (and table name, incidentally, so the separate name isn't even necessary here).  Anything else is a TOCTOU violation that the user can't work around.  Now, if the API took _only_ a table name (and not the table object), I could understand sending the RPC without an ID.


-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Hello Todd Lipcon,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/8026

to look at the new patch set (#4).

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................

KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

This patch modifies new Java clients to no longer use the create_table_done
field in GetTableSchema RPCs. After the CreateTable RPC returns, the client
will issue a GetTableSchema RPC to build a KuduTable, then enter an
IsCreateTableDone RPC loop until all of the tablets have been created.

There are a couple other changes worth noting:
- openTable no longer waits for table creation before returning. Doing that
  now would require at least one IsCreateTableDone RPC, which basically
  defeats the purpose of these changes. Moreover, the C++ client doesn't do
  this, so I don't see why the Java client should.
- I removed the 'tablesNotServed' logic which had no useful effect because,
  as I recently learned, there's retry logic to deal with TABLET_NOT_RUNNING
  master errors embedded more deeply in the Java client.
- I removed the master permit acquisition from the "is create table done"
  loop, since these loops should no longer produce a thundering herds (as I
  suppose they could have due to 'tablesNotServed').
- I modified createTable and alterTable to make waiting optional. The
  default behavior is to wait, which was already the case for createTable,
  but not for alterTable. I also added an isCreateTableDone method, which is
  now somewhat useful as createTable's waiting is optional.
- The IsCreateTableDone and IsAlterTableDone loops now use table IDs to
  ensure that they're robust in the face of concurrent operations that may
  change table names. This depends on AlterTableResponsePB containing table
  IDs, which was only true as of Kudu 0.10. If used against an older server,
  the client will throw an exception.

Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.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/CreateTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeadlineTracker.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java
A java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestDeadlineTracker.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
18 files changed, 694 insertions(+), 405 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/8026/4
-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................


Patch Set 5: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................


Patch Set 1:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/8026/1//COMMIT_MSG
Commit Message:

PS1, Line 23: Meanwhile
> I read this to mean that this patch is both modifying the master as describ
I'll try to clarify while still providing context to understand the motivation for this patch.


http://gerrit.cloudera.org:8080/#/c/8026/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:

Line 524:    * Gets a table's schema
> Missing period
Done


http://gerrit.cloudera.org:8080/#/c/8026/1/java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java
File java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java:

Line 36:   IsCreateTableDoneRequest(KuduTable table, String name) {
> Doesn't identifying the table by name open the client up to a race conditio
I suppose so, but IsAlterTableDoneRequest behaves this way, as do both IsAlterTableDone and IsCreateTableDone on the C++ side. Moreover, in the standalone case (i.e. if a user just makes those client calls), the table name is all you have anyway.

So I'd prefer to keep them consistent.


http://gerrit.cloudera.org:8080/#/c/8026/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java:

Line 99:   public boolean isCreateTableDone(String name) throws KuduException {
> Why is this not a joinAndHandleException wrapper around an async variant?
I was copying isAlterTableDone without thinking about it too hard. In order to actually wait for the creation to finish, joining the deferred is insufficient; we need a loop that sleeps and retries too.


Line 101:     while (totalSleepTime < getDefaultAdminOperationTimeoutMs()) {
> This implementation appears to be waiting for the table to be created if it
I don't want to change the method name (again, to be consistent with IsAlterTableDone), but I will reword the Javadocs to make this behavior more clear.


Line 121:           LOG.debug("Create not done, sleep " + (AsyncKuduClient.SLEEP_TIME - elapsed) +
> This log message doesn't make a lot of sense in isolation, and restructure 
I think it's OK that it doesn't make much sense since it's debug-level (something we'd normally enable just for isolated testing).

I will restructure it (and the version in IsAlterTableDone) though.


Line 132:     return false;
> Shouldn't this throw a timeout exception?
That's not the behavior of IsAlterTableDone, and I was trying to be consistent with it.


http://gerrit.cloudera.org:8080/#/c/8026/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:

Line 940:             if (e.getMessage().contains("does not exist")) {
> I think this could be more precise by asserting on the returned Status (Not
Done


Line 954:           Insert insert = table.newInsert();
> Shouldn't this work even without waiting for isCreateTableDone?  I would ha
Yes, we discussed this yesterday. Will make the change.


-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................


Patch Set 3: Verified+1

Overriding Jenkins, the sole failure was in setting up one of the TSAN tests:

  INFO    142   isolateserver(1843): Retrieving remaining files (65 of them)...
  ERROR  18226   isolateserver(599): Failed to fetch 87cf40e5c6969e2c3a07236da9d950dd693ea974: [Errno 104] Connection reset by peer

-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................


Patch Set 3:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/8026/3/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 361:    * considered to be successful.
> What happens if you elect to not wait? What's the expectation here? Right n
If you don't wait, you may operate on the table before the alter is finished. Depending on what you're altering, the operation may or may not be impacted:
- A renamed table will have no discernible effect.
- Inserts to an added range partition will retry until that partition is created (so no effect).
- A changed schema (i.e. add/modify/drop column) won't be propagated to clients in openTable() until the alter is done.

Not waiting is useful if you want to alter a lot of tables as quickly as possible (i.e. as an admin tool). I also think it's good to have just for feature parity with the C++ client.

FWIW, the C++ client documents the 'wait' functionality of creating/altering a table rather vaguely since the specifics are definitely "implementation detail" level.


http://gerrit.cloudera.org:8080/#/c/8026/3/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 595:       String tableId,
> you think its' worth adding @Nullable to this and @Nonnull to 'tableName' f
I didn't know we used those annotations, but I see a couple other places in the Java client where we do. Sure, why not.


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
File java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java:

Line 191:    * Whether to wait for all tablets to be created before the table creation is
> What happens if you set this to false? Is your client going to blow up when
It's hard to doc this completely without mentioning some implementation details. The C++ client is even more vague, saying that wait(true) means to wait "until the table is fully created" before returning. I think it's reasonable to mention the existence of "tablets", though. That's what I documented in AlterTableOptions.

As for your other questions:
1. The client won't blow up if you use the table, though certain operations may retry under the hood.
2. I think this is useful for batch creation of many tables without explicitly waiting for any of them, and for feature parity with the C++ client.

As an aside, I do appreciate your questions about how to communicate this stuff to users, and if you have concrete doc suggestions I'll definitely consider them.


PS3, Line 199: setWait
> I disagree.  It's just called 'wait' in the C++ API.
I agree with Dan; I think being more vague is better in this case. If we're too specific, we open the door for confusion down the road if/when we want to tack on additional sub-operations to CreateTable and "tablet assignment" or whatever we've written here no longer captures it all.


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
File java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java:

Line 58:       Preconditions.checkNotNull(name);
> This precondition is already checked in the constructor.
Yes, but this is now moot: I reworked this stuff to take a TableIdentifierPB.Builder in the constructor (to better encapsulate the "name vs. id" dichotomy).


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java
File java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java:

Line 57:       Preconditions.checkNotNull(name);
> Likewise.
Moot for the same reason.


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java
File java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java:

Line 78:       Preconditions.checkNotNull(name);
> Likewise
Moot.


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java:

PS3, Line 97: isCreateTableDone
> this seems misnamed -- shouldn't it be 'waitForCreateTableCompletion' or so
Agreed, but that ship has sailed on isAlterTableDone, and I'd rather the two be consistent with one another than one have a more correct name. I'm hoping the Javadoc will help here.


Line 99:     while (timeSleptMillis < getDefaultAdminOperationTimeoutMs()) {
> do you think it's possible to factor out this "retry running an async metho
I did you one better: I refactored the asynchronous IsFooTableDone RPC loop so that it's usable from here, and dropped these handwritten synchronous loops entirely.


PS3, Line 166: isAlterTableDone
> same, although I guess this one was already implemented, so it's hard to re
Right, and because this one has a crappy name, I've opted to give the new one a crappy name too, for Consistency (tm).


http://gerrit.cloudera.org:8080/#/c/8026/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:

Line 947:     Future<?> fut = exec.submit(new Runnable() {
> why not just use deferreds / the async client?
Sure, why not.


-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Hello Dan Burkert, Jean-Daniel Cryans, Todd Lipcon, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/8026

to look at the new patch set (#3).

Change subject: KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients
......................................................................

KUDU-1807 (part 2): ban GetTableSchema for table createdness in clients

This patch modifies new Java clients to no longer use the create_table_done
field in GetTableSchema RPCs. After the CreateTable RPC returns, the client
will issue a GetTableSchema RPC to build a KuduTable, then enter an
IsCreateTableDone RPC loop until all of the tablets have been created.

There are a couple other changes worth noting:
- openTable no longer waits for table creation before returning. Doing that
  now would require at least one IsCreateTableDone RPC, which basically
  defeats the purpose of these changes. Moreover, the C++ client doesn't do
  this, so I don't see why the Java client should.
- I removed the 'tablesNotServed' logic which had no useful effect because,
  as I recently learned, there's retry logic to deal with TABLET_NOT_RUNNING
  master errors embedded more deeply in the Java client.
- I removed the master permit acquisition from the "is create table done"
  loop, since these loops should no longer produce a thundering herds (as I
  suppose they could have due to 'tablesNotServed').
- I modified createTable and alterTable to make waiting optional. The
  default behavior is to wait, which was already the case for createTable,
  but not for alterTable. I also added an isCreateTableDone method, which is
  now somewhat useful as createTable's waiting is optional.
- The IsCreateTableDone and IsAlterTableDone loops now use table IDs to
  ensure that they're robust in the face of concurrent operations that may
  change table names. This depends on AlterTableResponsePB containing table
  IDs, which was only true as of Kudu 0.10. If used against an older server,
  the client will throw an exception.

Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.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/CreateTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeadlineTracker.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java
A java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestDeadlineTracker.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
18 files changed, 687 insertions(+), 377 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/8026/3
-- 
To view, visit http://gerrit.cloudera.org:8080/8026
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54fa07dc34a97f1c9da06ec9129d6d4590b7aac6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>