[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


KUDU-2138 delete failed replicas in tablet report

When a Kudu master processes a replica's tablet report, it will send a
DeleteTablet RPC if the replica is not already deleted, has a consensus
state, has a lower opid index than the latest reported committed config,
and is no longer in the tablet's Raft configuration.

Failed tablets are handled specially in processing the report to return
before any of the above checks can be made. This can lead to failed
tablets lingering when they should actually be deleted.

This patch fixes this by performing these checks and sending the delete
regardless of whether or not the tablet is failed (other than checking
the opid index, since failed tablets do not report an opid index).

A test is added to tablet_replacement-itest to fail a replica, evict but
not tombstone it, report it to the master, and ensure it is properly
deleted.

Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Reviewed-on: http://gerrit.cloudera.org:8080/7992
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy 
---
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 102 insertions(+), 23 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Hello Mike Percy, Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-2138 delete failed replicas in tablet report
..

KUDU-2138 delete failed replicas in tablet report

When a Kudu master processes a replica's tablet report, it will send a
DeleteTablet RPC if the replica is not already deleted, has a consensus
state, has a lower opid index than the latest reported committed config,
and is no longer in the tablet's Raft configuration.

Failed tablets are handled specially in processing the report to return
before any of the above checks can be made. This can lead to failed
tablets lingering when they should actually be deleted.

This patch fixes this by performing these checks and sending the delete
regardless of whether or not the tablet is failed (other than checking
the opid index, since failed tablets do not report an opid index).

A test is added to tablet_replacement-itest to fail a replica, evict but
not tombstone it, report it to the master, and ensure it is properly
deleted.

Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
---
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 102 insertions(+), 23 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:

PS4, Line 182: AS
> nit: extra indentation
Done


http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

PS4, Line 2488: Replic
> Replica?
Done


PS4, Line 2492: fig index is $1)", 
> how about: current committed config index is $1
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 4: Code-Review+1

(3 comments)

looks good, just a couple nits

http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:

PS4, Line 182:   
nit: extra indentation


http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

PS4, Line 2488: Tablet
Replica?


PS4, Line 2492: current index is $1
how about: current committed config index is $1


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 4: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-2138 delete failed replicas in tablet report
..

KUDU-2138 delete failed replicas in tablet report

When a Kudu master processes a replica's tablet report, it will send a
DeleteTablet RPC if the replica is not already deleted, has a consensus
state, has a lower opid index than the latest reported committed config,
and is no longer in the tablet's Raft configuration.

Failed tablets are handled specially in processing the report to return
before any of the above checks can be made. This can lead to failed
tablets lingering when they should actually be deleted.

This patch fixes this by performing these checks and sending the delete
regardless of whether or not the tablet is failed (other than checking
the opid index, since failed tablets do not report an opid index).

A test is added to tablet_replacement-itest to fail a replica, evict but
not tombstone it, report it to the master, and ensure it is properly
deleted.

Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
---
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 102 insertions(+), 23 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 4:

Just fixed IWYU failure, still good for review.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7992/2/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:

PS2, Line 184: an
> a
Done


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

Line 2590:   if (report.has_error()) {
> I'm curious as to why this block was moved down here and not just before th
I'd wanted it to be slightly tailored for the case I expect is more common 
where report.has_consensus_state() == true so I put it first. It's not really 
an important move, so I'll move it back, sorry about that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-2138 delete failed replicas in tablet report
..

KUDU-2138 delete failed replicas in tablet report

When a Kudu master processes a replica's tablet report, it will send a
DeleteTablet RPC if the replica is not already deleted, has a consensus
state, has a lower opid index than the latest reported committed config,
and is no longer in the tablet's Raft configuration.

Failed tablets are handled specially in processing the report to return
before any of the above checks can be made. This can lead to failed
tablets lingering when they should actually be deleted.

This patch fixes this by performing these checks and sending the delete
regardless of whether or not the tablet is failed (other than checking
the opid index, since failed tablets do not report an opid index).

A test is added to tablet_replacement-itest to fail a replica, evict but
not tombstone it, report it to the master, and ensure it is properly
deleted.

Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
---
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 101 insertions(+), 23 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 2: Code-Review+1

(2 comments)

Looks OK, but would prefer if Mike +2'ed.

http://gerrit.cloudera.org:8080/#/c/7992/2/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:

PS2, Line 184: an
a


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

Line 2590:   if (report.has_error()) {
I'm curious as to why this block was moved down here and not just before the 
large "if report.has_consensus_state()" block?

I can take my answer in the form of a code comment.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: KUDU-2138 delete failed replicas in tablet report
..


Patch Set 2:

IWYU failure; this patch is still reviewable

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new patch set (#2).

Change subject: KUDU-2138 delete failed replicas in tablet report
..

KUDU-2138 delete failed replicas in tablet report

When a Kudu master processes a replica's tablet report, it will send a
DeleteTablet RPC if the replica is not already deleted, has a consensus
state, has a lower opid index than the latest reported committed config,
and is no longer in the tablet's Raft configuration.

Failed tablets are handled specially in processing the report to return
before any of the above checks can be made. This can lead to failed
tablets lingering when they should actually be deleted.

This patch fixes this by performing these checks and sending the delete
regardless of whether or not the tablet is failed (other than checking
the opid index, since failed tablets do not report an opid index).

A test is added to tablet_replacement-itest to fail a replica, evict but
not tombstone it, report it to the master, and ensure it is properly
deleted.

Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
---
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 104 insertions(+), 28 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2138 delete failed replicas in tablet report

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new change for review.

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

Change subject: KUDU-2138 delete failed replicas in tablet report
..

KUDU-2138 delete failed replicas in tablet report

When a Kudu master processes a replica's tablet report, it will send a
DeleteTablet RPC if the replica is not already deleted, has a consensus
state, has a lower opid index than the latest reported committed config,
and is no longer in the tablet's Raft configuration.

Failed tablets are handled specially in processing the report to return
before any of the above checks can be made. This can lead to failed
tablets lingering when they should actually be deleted.

This patch fixes this by performing these checks and sending the delete
regardless of whether or not the tablet is failed (other than checking
the opid index, since failed tablets do not report an opid index).

A test is added to tablet_replacement-itest to fail a replica, evict but
not tombstone it, report it to the master, and ensure it is properly
deleted.

Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
---
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 109 insertions(+), 28 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong