You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/05/18 01:58:23 UTC
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6916
to review the following change.
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
This adds a flag on CowObject locks to track whether the mutable data
field has ever been accessed. In the case that it's not accessed, we can
guarantee that the underlying object didn't change, in which case it's
safe to avoid writing it to disk during a tablet report.
This is a simple way of short-circuiting a common case of unnecessary
slowness in tablet report processing.
I manually verified that, if I restarted a master with a single TS
reporting to it, when it came back up, it was able to skip writes to
disk when receiving tablet reports redundant with existing information.
This doesn't achieve all of the goals of KUDU-1125 (we still do separate
sync writes for each reported tablet) but should still be a substantial
reduction. Perhaps most importantly, if a heartbeat from a tserver times
out due to long processing, the retry from the TS should hit this code
path and therefore be processed quickly.
Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
---
M src/kudu/master/catalog_manager.cc
M src/kudu/util/cow_object.h
2 files changed, 36 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6916/1
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6916/3/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
Line 114: return md.Compare(prev_pb, new_pb);
I presume this is equivalent to prev_pb == new_pb? But you're going through the MessageDifferencer for the diff_str!= null case?
Bearing in mind that the diff_str!=null case is very rare (VLOG only), would it be faster to use equality? Or are the two approaches equivalent?
Line 692: VLOG(2) << "Adding tablet " << tablet->tablet_id() << " in catalog: "
Any particular reason you want this VLOG but not one for ReqAddTable()?
http://gerrit.cloudera.org:8080/#/c/6916/3/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
Line 164: const scoped_refptr<tablet::TabletReplica>& tablet_replica() const {
Since this is only being used in a test, an alternative would be to keep it private and add FRIEND_TEST() as needed.
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6916/1//COMMIT_MSG
Commit Message:
PS1, Line 17: I manually verified that, if I restarted a master with a single TS
: reporting to it, when it came back up, it was able to skip writes to
: disk when receiving tablet reports redundant with existing information.
Any way to automate the testing? Perhaps using catalog write metrics?
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Todd Lipcon (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/6916
to look at the new patch set (#4).
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
This adds some checks in SysCatalogTable so that, if the updated version
of a catalog table entry is identical to the previous version, we avoid
writing anything to the table.
This is a simple way of short-circuiting a common case of unnecessary
slowness in tablet report processing. For example, when the master
restarts or fails over, all of the tablet servers perform a full tablet
report. However, most of the tablets will not have changed any
information since their prior report, in which case the writes can be
safely skipped on the master.
This doesn't achieve all of the goals of KUDU-1125 (we still do separate
sync writes for each _changed_ reported tablet) but should still be a
substantial reduction. Perhaps most importantly, if a heartbeat from a
tserver times out due to long processing, the retry from the TS should
hit this code path and therefore be processed quickly.
Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test-util.h
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 106 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6916/4
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6916
to look at the new patch set (#3).
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
This adds some checks in SysCatalogTable so that, if the updated version
of a catalog table entry is identical to the previous version, we avoid
writing anything to the table.
This is a simple way of short-circuiting a common case of unnecessary
slowness in tablet report processing. For example, when the master
restarts or fails over, all of the tablet servers perform a full tablet
report. However, most of the tablets will not have changed any
information since their prior report, in which case the writes can be
safely skipped on the master.
This doesn't achieve all of the goals of KUDU-1125 (we still do separate
sync writes for each _changed_ reported tablet) but should still be a
substantial reduction. Perhaps most importantly, if a heartbeat from a
tserver times out due to long processing, the retry from the TS should
hit this code path and therefore be processed quickly.
Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test-util.h
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 103 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6916/3
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6916/2/src/kudu/integration-tests/registration-test.cc
File src/kudu/integration-tests/registration-test.cc:
Line 38: #include "kudu/tablet/tablet_replica.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/6916/2/src/kudu/master/master-test-util.h
File src/kudu/master/master-test-util.h:
Line 71: void CreateTableForTesting(MiniMaster* mini_master,
> warning: function 'CreateTableForTesting' defined in a header file; functio
this is only used by one test (registration-test). I'll move it into there in a separate commit.
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6916/3/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
Line 114: return md.Compare(prev_pb, new_pb);
> I presume this is equivalent to prev_pb == new_pb? But you're going through
I don't think Message or MessageLite implements operator==, best I can tell.
Line 692: VLOG(2) << "Adding tablet " << tablet->tablet_id() << " in catalog: "
> Any particular reason you want this VLOG but not one for ReqAddTable()?
oversight. will add.
http://gerrit.cloudera.org:8080/#/c/6916/3/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
Line 164: const scoped_refptr<tablet::TabletReplica>& tablet_replica() const {
> Since this is only being used in a test, an alternative would be to keep it
I couldn't do that, because the test it's being used in isn't in the 'master' namespace (I tried and failed)
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
This adds some checks in SysCatalogTable so that, if the updated version
of a catalog table entry is identical to the previous version, we avoid
writing anything to the table.
This is a simple way of short-circuiting a common case of unnecessary
slowness in tablet report processing. For example, when the master
restarts or fails over, all of the tablet servers perform a full tablet
report. However, most of the tablets will not have changed any
information since their prior report, in which case the writes can be
safely skipped on the master.
This doesn't achieve all of the goals of KUDU-1125 (we still do separate
sync writes for each _changed_ reported tablet) but should still be a
substantial reduction. Perhaps most importantly, if a heartbeat from a
tserver times out due to long processing, the retry from the TS should
hit this code path and therefore be processed quickly.
Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Reviewed-on: http://gerrit.cloudera.org:8080/6916
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test-util.h
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 106 insertions(+), 17 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6916
to look at the new patch set (#2).
Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
......................................................................
KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info
This adds some checks in SysCatalogTable so that, if the updated version
of a catalog table entry is identical to the previous version, we avoid
writing anything to the table.
This is a simple way of short-circuiting a common case of unnecessary
slowness in tablet report processing. For example, when the master
restarts or fails over, all of the tablet servers perform a full tablet
report. However, most of the tablets will not have changed any
information since their prior report, in which case the writes can be
safely skipped on the master.
This doesn't achieve all of the goals of KUDU-1125 (we still do separate
sync writes for each _changed_ reported tablet) but should still be a
substantial reduction. Perhaps most importantly, if a heartbeat from a
tserver times out due to long processing, the retry from the TS should
hit this code path and therefore be processed quickly.
Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test-util.h
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 103 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6916/2
--
To view, visit http://gerrit.cloudera.org:8080/6916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins