[kudu-CR] master: additional leader lock assertions in catalog manager

2016-08-04 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: master: additional leader lock assertions in catalog manager
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bb2f5067cdbdd93900a80255def65a26216f6ea
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] master: additional leader lock assertions in catalog manager

2016-08-04 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

Change subject: master: additional leader lock assertions in catalog manager
..


Patch Set 6:

Build Started http://104.196.14.100/job/kudu-gerrit/2714/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bb2f5067cdbdd93900a80255def65a26216f6ea
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] master: additional leader lock assertions in catalog manager

2016-08-04 Thread Adar Dembo (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: master: additional leader lock assertions in catalog manager
..

master: additional leader lock assertions in catalog manager

I went through the catalog manager entry points and added leader lock
assertions where necessary, updating tests as needed.

I also snuck in a couple cluster fixes:
1. MiniCluster::leader_mini_master() was unsafe because it didn't pass the
   (now held) leader lock back to the caller. It's only used in a few places
   though, so I removed it outright rather than fix it.
2. WaitForTabletServerCount() has been updated for both kinds of clusters.
   The new version waits for the correct count on every master, a necessary
   change now that tservers heartbeat to every master. Without this, we may
   stop waiting when the master that has seen all tservers was a follower
   and fail a subsequent CreateTable. The new version also ignore masters
   that have been shut down. This isn't essential, but good future-proofing.

Change-Id: I5bb2f5067cdbdd93900a80255def65a26216f6ea
---
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/integration-tests/mini_cluster.h
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
10 files changed, 155 insertions(+), 109 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/3684/6
-- 
To view, visit http://gerrit.cloudera.org:8080/3684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bb2f5067cdbdd93900a80255def65a26216f6ea
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] master: additional leader lock assertions in catalog manager

2016-08-04 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: master: additional leader lock assertions in catalog manager
..


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3684/5/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:

Line 470:   // Return true if the table with the specified ID exists,
> doesn't return bool
Done


Line 473:   Status GetTableInfo(const std::string& table_id, 
scoped_refptr *table);
> should update docs to explain bad status results
Done


Line 477:   Status GetAllTables(std::vector* tables);
> same here
Done


Line 479:   // Return true if the specified table name exists
> same
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bb2f5067cdbdd93900a80255def65a26216f6ea
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] master: additional leader lock assertions in catalog manager

2016-08-03 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: master: additional leader lock assertions in catalog manager
..


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3684/5/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:

Line 470:   // Return true if the table with the specified ID exists,
doesn't return bool


Line 473:   Status GetTableInfo(const std::string& table_id, 
scoped_refptr *table);
should update docs to explain bad status results

also, should doc that these methods require the caller to hold the leader lock


Line 477:   Status GetAllTables(std::vector* tables);
same here


Line 479:   // Return true if the specified table name exists
same


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bb2f5067cdbdd93900a80255def65a26216f6ea
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] master: additional leader lock assertions in catalog manager

2016-08-02 Thread Kudu Jenkins (Code Review) 
Kudu Jenkins has posted comments on this change.

Change subject: master: additional leader lock assertions in catalog manager
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/2705/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bb2f5067cdbdd93900a80255def65a26216f6ea
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] master: additional leader lock assertions in catalog manager

2016-08-02 Thread Adar Dembo (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: master: additional leader lock assertions in catalog manager
..

master: additional leader lock assertions in catalog manager

I went through the catalog manager entry points and added leader lock
assertions where necessary, updating tests as needed.

I also snuck in a couple cluster fixes:
1. MiniCluster::leader_mini_master() was unsafe because it didn't pass the
   (now held) leader lock back to the caller. It's only used in a few places
   though, so I removed it outright rather than fix it.
2. WaitForTabletServerCount() has been updated for both kinds of clusters.
   The new version waits for the correct count on every master, a necessary
   change now that tservers heartbeat to every master. Without this, we may
   stop waiting when the master that has seen all tservers was a follower
   and fail a subsequent CreateTable. The new version also ignore masters
   that have been shut down. This isn't essential, but good future-proofing.

Change-Id: I5bb2f5067cdbdd93900a80255def65a26216f6ea
---
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/integration-tests/mini_cluster.h
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-path-handlers.cc
10 files changed, 146 insertions(+), 105 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/3684/5
-- 
To view, visit http://gerrit.cloudera.org:8080/3684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bb2f5067cdbdd93900a80255def65a26216f6ea
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon