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:34 UTC
[kudu-CR] KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
Hello Dan Burkert, Jean-Daniel Cryans, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8027
to review the following change.
Change subject: KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
......................................................................
KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
At first I tried to optimize IsCreateInProgress by caching some additional
in-memory state, but ran into a thicket of locking issues. So here's an
alternative approach: deprecate GetTableSchemaResponsePB's create_table_done
field and always set it to false.
AFAICT, the C++ client never referenced this field. The Java client does; it
uses it to decide if a table is fully created during table open. However, if
create_table_done is false, it waits further via a IsCreateTableDone RPC
loop. This means we can safely set create_table_done to false without
breaking compatibility with old Java clients; they'll compensate by issuing
an additional IsCreateTableDone RPC when opening a table.
Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
3 files changed, 3 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/8027/1
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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 1): deprecate GetTableSchema.create table done
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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): stop calling IsCreateInProgress in GetTableSchema
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
......................................................................
Patch Set 1: Verified+1
Overriding Jenkins, the only failure was KUDU-2109.
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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: No
[kudu-CR] KUDU-1807 (part 1): deprecate GetTableSchema.create table done
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8027
to look at the new patch set (#3).
Change subject: KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
......................................................................
KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
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.
Future patches will change the behavior of the Java client and remove the
create_table_done field altogether. For now, the existence of this
deprecation patch helps to ensure that the Java client does, in fact, work
properly if the field's value is always false.
Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
3 files changed, 3 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/8027/3
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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>
[kudu-CR] KUDU-1807 (part 1): deprecate GetTableSchema.create table done
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8027/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:
PS1, Line 583: false
> did you miss this nit?
Sorry, I did.
The value is always unset. I'll clarify.
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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 1): deprecate GetTableSchema.create table done
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8027
to look at the new patch set (#2).
Change subject: KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
......................................................................
KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
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.
Future patches will change the behavior of the Java client and remove the
create_table_done field altogether. For now, the existence of this
deprecation patch helps to ensure that the Java client does, in fact, work
properly if the field's value is always false.
Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
3 files changed, 3 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/8027/2
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1807 (part 1): deprecate GetTableSchema.create table done
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
......................................................................
KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
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.
Future patches will change the behavior of the Java client and remove the
create_table_done field altogether. For now, the existence of this
deprecation patch helps to ensure that the Java client does, in fact, work
properly if the field's value is always false.
Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
Reviewed-on: http://gerrit.cloudera.org:8080/8027
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
3 files changed, 3 insertions(+), 16 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8027/1//COMMIT_MSG
Commit Message:
Line 18: breaking compatibility with old Java clients; they'll compensate by issuing
Shouldn't this be part 1? Making this part 1 would at least give us some amount of confidence that the existing Java client can interoperate with this change.
http://gerrit.cloudera.org:8080/#/c/8027/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:
Line 585: optional bool DEPRECATED_create_table_done = 6;
Instead of renaming, change to:
optional bool create_table_done = 6 [deprecated=true];
Also, consider adding a part 3 which removes the field and adds 6 as a reserved tag (https://developers.google.com/protocol-buffers/docs/proto#reserved).
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8027/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:
PS1, Line 583: false
is it always false or always unset?
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1807 (part 2): stop calling IsCreateInProgress in GetTableSchema
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8027/1//COMMIT_MSG
Commit Message:
Line 18: breaking compatibility with old Java clients; they'll compensate by issuing
> Shouldn't this be part 1? Making this part 1 would at least give us some a
Done
http://gerrit.cloudera.org:8080/#/c/8027/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:
Line 585: optional bool DEPRECATED_create_table_done = 6;
> Instead of renaming, change to:
Done
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1807 (part 1): deprecate GetTableSchema.create table done
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1807 (part 1): deprecate GetTableSchema.create_table_done
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8027/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:
PS1, Line 583: false
> is it always false or always unset?
did you miss this nit?
--
To view, visit http://gerrit.cloudera.org:8080/8027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia33bbb2abaabb97db1613d442a7d065710048cc2
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