[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9052 )

Change subject: WIP [master] no half-baked responses on ConnectoToMaster
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 
'role' is LEADER but
> Sorry about being fuzzy.  Agreed, that's confusing.  Maybe, I should handle
It should have been '... current code will make ...' instead of '... correct 
code will make ...'.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 19 Jan 2018 23:22:45 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9052 )

Change subject: WIP [master] no half-baked responses on ConnectoToMaster
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@463
PS2, Line 463: // Do not send half-baked responses from leader master when 
its catalog
 : // manager is not in proper state yet.
> So it seems like we thought we had a world with the two states { FOLLOWER,
I think that makes sense, and some time ago there was a patch which did 
something similar:

https://gerrit.cloudera.org/#/c/6356/

I think I'll do like you suggested.  I think that's a good idea especially 
given the fact that the clients are supposed to handle such cases.  Thanks!


http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 
'role' is LEADER but
> Hmm, but the message is consistent with the bug you're addressing ("a short
Sorry about being fuzzy.  Agreed, that's confusing.  Maybe, I should handle 
KUDU-1924 in this commit as well.  Basically, I think we can apply your idea 
here as well: send the role of the server as FOLLOWER in the response if this 
current leader doesn't have proper CA cert or TSK yet.

BTW, the correct code will make the process crash if leader_status.ok() but the 
CA cert hasn't been set yet.  That's because of CHECK() in the 
MasterCertAuthority::ca_cert_der() method.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 19 Jan 2018 23:18:41 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9052 )

Change subject: WIP [master] no half-baked responses on ConnectoToMaster
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@463
PS2, Line 463: // Do not send half-baked responses from leader master when 
its catalog
 : // manager is not in proper state yet.
> I'm not sure this is by design.  My point is that in case if a leader is no
So it seems like we thought we had a world with the two states { FOLLOWER, 
LEADER } but we actually have a world with the three states { FOLLOWER, 
FOLLOWER_TRANSITIONING_TO_LEADER, LEADER }. This patch identifies the new 
state, but what I don't like about it is that it returns an error while in that 
state when no error is returned while in the other two states.

Instead of doing this, what if we treated role == LEADER && !leader_status.ok() 
the same as role == FOLLOWER? That is, do resp->set_role(FOLLOWER), do 
resp->add_master_addrs() as before, and skip the KUDU-1924 stuff below? It's 
not "wrong" per se; it just means this master will be considered a FOLLOWER by 
the client during the (short) transition to LEADER. Clients should already 
handle the scenario when all masters are FOLLOWER because they're doing a 
leader election; this just prolongs that state slightly.


http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 
'role' is LEADER but
> That's about some different case, actually -- when leader_status.ok() but n
Hmm, but the message is consistent with the bug you're addressing ("a short 
window wherein the master is LEADER but isn't done initializing").



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 19 Jan 2018 22:32:50 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9052 )

Change subject: WIP [master] no half-baked responses on ConnectoToMaster
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@463
PS2, Line 463: // Do not send half-baked responses from leader master when 
its catalog
 : // manager is not in proper state yet.
> Are you sure that's not by design? Todd added ConnectToMaster and I think t
I'm not sure this is by design.  My point is that in case if a leader is not in 
OK state, it's not usable as a leader anyway.  Why to report such server as a 
leader master if it cannot be used as a leader master in the sense that client 
assumes it to be the case if it gets success response marked "I'm the leader 
master"?

The only useful info from this method in that case seems to be the set of other 
master servers' end-points.  Do you mean that information should always be 
included into the response?  But then I don't understand why we don't include 
that in the case if code at line 456 fires.


http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 
'role' is LEADER but
> This comment seems relevant.
That's about some different case, actually -- when leader_status.ok() but no CA 
cert or TSK has been generated yet.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 19 Jan 2018 22:19:50 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9052 )

Change subject: WIP [master] no half-baked responses on ConnectoToMaster
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@463
PS2, Line 463: // Do not send half-baked responses from leader master when 
its catalog
 : // manager is not in proper state yet.
Are you sure that's not by design? Todd added ConnectToMaster and I think the 
intent was for it to always "succeed" and supply enough information to explain 
the state the master was in.


http://gerrit.cloudera.org:8080/#/c/9052/2/src/kudu/master/master_service.cc@488
PS2, Line 488: // TODO(KUDU-1924): it seems there is some window when 
'role' is LEADER but
This comment seems relevant.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 18 Jan 2018 21:24:11 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9052 )

Change subject: WIP [master] no half-baked responses on ConnectoToMaster
..


Patch Set 2: Verified+1

Unrelated flaek in 
BlockManagerType/TsRecoveryITest.TestRestartWithPendingCommitFromFailedOp/1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 18 Jan 2018 20:17:42 +
Gerrit-HasComments: No

[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster

[Alexey Serbin (Code Review)]

Alexey Serbin has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/9052 )

Change subject: WIP [master] no half-baked responses on ConnectoToMaster
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: WIP [master] no half-baked responses on ConnectoToMaster
..

WIP [master] no half-baked responses on ConnectoToMaster

Do not send half-baked responses to clients from master that declares
itself a leader when its catalog manager is not yet in proper state.

WIP: already existing AuthTokenIssuingTest.ChannelConfidentiality test
 provides some coverage for the new code, but maybe a dedicated
 test is needed?

Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/master_service.cc
2 files changed, 15 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9052/2
--
To view, visit http://gerrit.cloudera.org:8080/9052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP [master] no half-baked responses on ConnectoToMaster

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9052


Change subject: WIP [master] no half-baked responses on ConnectoToMaster
..

WIP [master] no half-baked responses on ConnectoToMaster

Do not send half-baked responses to clients from master that declares
itself a leader when its catalog manager is not yet in proper state.

WIP: already existing AuthTokenIssuingTest.ChannelConfidentiality test
 provides some coverage for the new code, but maybe a dedicated
 test is needed?

Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/master/master_service.cc
2 files changed, 14 insertions(+), 20 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4
Gerrit-Change-Number: 9052
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin