[kudu-CR] [master] add feature flag for 3-4-3 scheme

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9565 )

Change subject: [master] add feature flag for 3-4-3 scheme
..

[master] add feature flag for 3-4-3 scheme

This changelist addresses the case of misconfiguration of the
replica management scheme between masters and tablet servers
in the case when a master of version < 1.7 happen to work with
tablet servers of version >= 1.7.

Also, added a new run-time flag --heartbeat_incompatibility_is_fatal
to control whether the replica management scheme misconfiguration
should be treated as a fatal error for a tablet server.

This is a follow-up for 1769eed53ee2c21a88a766cb72bf8c9ae622099d.

Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
Reviewed-on: http://gerrit.cloudera.org:8080/9565
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/consensus/replica_management.proto
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tserver/heartbeater.cc
5 files changed, 112 insertions(+), 32 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Dan Burkert: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
Gerrit-Change-Number: 9565
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [master] add feature flag for 3-4-3 scheme

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9565 )

Change subject: [master] add feature flag for 3-4-3 scheme
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
Gerrit-Change-Number: 9565
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 09 Mar 2018 21:22:51 +
Gerrit-HasComments: No

[kudu-CR] [master] add feature flag for 3-4-3 scheme

[Alexey Serbin (Code Review)]

Hello Mike Percy, Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [master] add feature flag for 3-4-3 scheme
..

[master] add feature flag for 3-4-3 scheme

This changelist addresses the case of misconfiguration of the
replica management scheme between masters and tablet servers
in the case when a master of version < 1.7 happen to work with
tablet servers of version >= 1.7.

Also, added a new run-time flag --heartbeat_incompatibility_is_fatal
to control whether the replica management scheme misconfiguration
should be treated as a fatal error for a tablet server.

This is a follow-up for 1769eed53ee2c21a88a766cb72bf8c9ae622099d.

Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
---
M src/kudu/consensus/replica_management.proto
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tserver/heartbeater.cc
5 files changed, 112 insertions(+), 32 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
Gerrit-Change-Number: 9565
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [master] add feature flag for 3-4-3 scheme

[Alexey Serbin (Code Review)]

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

Change subject: [master] add feature flag for 3-4-3 scheme
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/consensus/replica_management.proto
File src/kudu/consensus/replica_management.proto:

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/consensus/replica_management.proto@27
PS2, Line 27: UNKNOWN = 999;
> The recommended style for new enums is to have the UNKNOWN variant be 0, an
Thank you for the clarification.  Unfortunately, that's already released with 
Kudu 1.6, so I'm not updating this enum.  However, for future enums I'll strive 
to use the recommended style.


http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master.proto@80
PS2, Line 80: INCOMPATIBLE_REPLICA_MANAGEMENT = 12;
> Big +1 on this, thanks!
Yep, I also think that having more specific error codes is better.  Thank for 
mentioning this on the Slack channel.


http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master.proto@715
PS2, Line 715:   PREPARE_REPLACEMENT_BEFORE_EVICTION = 4;
> Consider making this feature flag REPLICA_MANAGEMENT, since it's really jus
Indeed -- that would be more generic and will work as described, good 
observation!


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

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master_service.cc@164
PS2, Line 164: if (scheme != ts_scheme) {
> Circling back to my comment in master.proto, this code is written correctly
Indeed.


http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/tserver/heartbeater.cc@435
PS2, Line 435: if (info->replacement_scheme() ==
> Make this info->replacement_schema() != EVICT_FIRST, in case we ever add an
Yep, that would be more correct and cleaner.  Done.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
Gerrit-Change-Number: 9565
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 09 Mar 2018 20:50:04 +
Gerrit-HasComments: Yes

[kudu-CR] [master] add feature flag for 3-4-3 scheme

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9565 )

Change subject: [master] add feature flag for 3-4-3 scheme
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/tserver/heartbeater.cc@435
PS2, Line 435: if (info->replacement_scheme() ==
Make this info->replacement_schema() != EVICT_FIRST, in case we ever add 
another replacement scheme type.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
Gerrit-Change-Number: 9565
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 09 Mar 2018 17:56:45 +
Gerrit-HasComments: Yes

[kudu-CR] [master] add feature flag for 3-4-3 scheme

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9565 )

Change subject: [master] add feature flag for 3-4-3 scheme
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/consensus/replica_management.proto
File src/kudu/consensus/replica_management.proto:

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/consensus/replica_management.proto@27
PS2, Line 27: UNKNOWN = 999;
The recommended style for new enums is to have the UNKNOWN variant be 0, and 
count up from there.  It's recommended because that's the only way enums can be 
defined in proto3, and if we ever want to move to proto3 it will make the 
transition easier.  We have many cases of UNKNOWN = 999 in the codebase because 
we later went back and added UNKNOWN after we already had variants assigned to 
0.

I recommend changing this enum, but only if we haven't already released a 
version containing it, since the change is backwards incompatible.


http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master.proto@80
PS2, Line 80: INCOMPATIBLE_REPLICA_MANAGEMENT = 12;
Big +1 on this, thanks!


http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master.proto@715
PS2, Line 715:   PREPARE_REPLACEMENT_BEFORE_EVICTION = 4;
Consider making this feature flag REPLICA_MANAGEMENT, since it's really just a 
requirement that the master recognize and deal with the replica_management_info 
field.

If in the future a new replica replacement policy type is added, we don't 
actually need a new feature flag.  In that scenario we'd add the policy type to 
the ReplacementScheme enum, the tserver would send the value to the master, and 
the master would recognize that it doesn't understand the policy type because 
it would deserialize it as UNKNOWN.


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

http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master_service.cc@164
PS2, Line 164: if (scheme != ts_scheme) {
Circling back to my comment in master.proto, this code is written correctly to 
handle UNKNOWN.  In that case we fall into this block and return 
INCOMPATIBLE_REPLICA_MANAGEMENT, which is good.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
Gerrit-Change-Number: 9565
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 09 Mar 2018 17:53:04 +
Gerrit-HasComments: Yes

[kudu-CR] [master] add feature flag for 3-4-3 scheme

[Alexey Serbin (Code Review)]

Hello Mike Percy, Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [master] add feature flag for 3-4-3 scheme
..

[master] add feature flag for 3-4-3 scheme

This changelist addresses the case of misconfiguration of the
replica management scheme between masters and tablet servers
in the case when a master of version < 1.7 happen to work with
tablet servers of version >= 1.7.

Also, added a new run-time flag --heartbeat_incompatibility_is_fatal
to control whether the replica management scheme misconfiguration
should be treated as a fatal error for a tablet server.

This is a follow-up for 1769eed53ee2c21a88a766cb72bf8c9ae622099d.

Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
---
M src/kudu/consensus/replica_management.proto
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tserver/heartbeater.cc
5 files changed, 111 insertions(+), 32 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
Gerrit-Change-Number: 9565
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [master] add feature flag for 3-4-3 scheme

[Alexey Serbin (Code Review)]

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


Change subject: [master] add feature flag for 3-4-3 scheme
..

[master] add feature flag for 3-4-3 scheme

This changelist addresses the case of misconfiguration of the
replica management scheme between masters and tablet servers
in the case when a master of version < 1.7 happen to work with
tablet servers of version >= 1.7.

Also, added a new run-time flag --heartbeat_incompatibility_is_fatal
to control whether the replica management scheme misconfiguration
should be treated as a fatal error for a tablet server.

This is a follow-up for 1769eed53ee2c21a88a766cb72bf8c9ae622099d.

Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9
---
M src/kudu/consensus/replica_management.proto
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tserver/heartbeater.cc
5 files changed, 103 insertions(+), 27 deletions(-)



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

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