You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/11/29 12:38:24 UTC
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8683
to review the following change.
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
KUDU-1097. master: strip health reports from cstate before persisting
The master should not persist configs with health reports in them
because the health reports are inherently transient information.
This patch strips the health reports from the cstate included in each
leader tablet report before persisting them.
Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
---
M src/kudu/master/catalog_manager.cc
1 file changed, 11 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/8683/1
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8683 )
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8683/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8683/1/src/kudu/master/catalog_manager.cc@3466
PS1, Line 3466: it
> nit: drop
Done
http://gerrit.cloudera.org:8080/#/c/8683/1/src/kudu/master/catalog_manager.cc@3474
PS1, Line 3474: if (peer->has_health_report())
> nit: drop this for clarity since clear_health_report() is idempotent
Done
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 30 Nov 2017 05:23:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8683 )
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8683/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8683/5/src/kudu/master/catalog_manager.cc@4054
PS5, Line 4054: DCHECK(!peer.has_health_report()); // Health report shouldn't be persisted.
It's a good one!
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 5
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 22:41:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8683 )
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8683/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8683/1/src/kudu/master/catalog_manager.cc@3466
PS1, Line 3466: it
nit: drop
http://gerrit.cloudera.org:8080/#/c/8683/1/src/kudu/master/catalog_manager.cc@3474
PS1, Line 3474: if (peer->has_health_report())
nit: drop this for clarity since clear_health_report() is idempotent
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Nov 2017 19:56:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8683 )
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
Patch Set 5:
this is just a rebase except for the additional DCHECKs
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 5
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 22:13:08 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#2) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/8683 )
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
KUDU-1097. master: strip health reports from cstate before persisting
The master should not persist configs with health reports in them
because the health reports are inherently transient information.
This patch strips the health reports from the cstate included in each
leader tablet report before persisting them.
Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
---
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 22 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/8683/2
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8683 )
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 6
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 02 Dec 2017 01:31:08 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#3) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/8683 )
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
KUDU-1097. master: strip health reports from cstate before persisting
The master should not persist configs with health reports in them
because the health reports are inherently transient information.
This patch strips the health reports from the cstate included in each
leader tablet report before persisting them.
Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
---
M src/kudu/master/catalog_manager.cc
1 file changed, 10 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/8683/3
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8683 )
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
KUDU-1097. master: strip health reports from cstate before persisting
The master should not persist configs with health reports in them
because the health reports are inherently transient information.
This patch strips the health reports from the cstate included in each
leader tablet report before persisting them and adds a couple of DCHECKS
in random places to enforce that.
Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Reviewed-on: http://gerrit.cloudera.org:8080/8683
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/master/catalog_manager.cc
1 file changed, 11 insertions(+), 1 deletion(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 7
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1097. master: strip health reports from cstate before persisting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8683
to look at the new patch set (#4).
Change subject: KUDU-1097. master: strip health reports from cstate before persisting
......................................................................
KUDU-1097. master: strip health reports from cstate before persisting
The master should not persist configs with health reports in them
because the health reports are inherently transient information.
This patch strips the health reports from the cstate included in each
leader tablet report before persisting them and adds a couple of DCHECKS
in random places to enforce that.
Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
---
M src/kudu/master/catalog_manager.cc
1 file changed, 11 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/8683/4
--
To view, visit http://gerrit.cloudera.org:8080/8683
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Gerrit-Change-Number: 8683
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins