You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2018/07/24 14:33:28 UTC
[kudu-CR] [tablet] clean-up on replay of WAL entries
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11032
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
[tablet] clean-up on replay of WAL entries
Reduced number of memory allocations while reading WAL entries from
disk during tablet bootstrap.
This changelist also contains other changes in tablet_bootstrap.cc
and corresponding WAL utilities:
* simplified ownership of log entries read from disk
* replaced gscoped_ptr with std::unique_ptr
* other minor updates
Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
---
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_local_replica.cc
11 files changed, 254 insertions(+), 238 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/11032/1
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [tablet] clean-up on replay of WAL entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11032 )
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
[tablet] clean-up on replay of WAL entries
Simplified handling of the ownership of log entries read from disk.
This changelist also contains other changes in tablet_bootstrap.cc
and corresponding WAL utilities:
* reduced number of memory allocations while reading WAL entries
* replaced gscoped_ptr with std::unique_ptr
* other minor updates around WAL-related utilities and tests
Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Reviewed-on: http://gerrit.cloudera.org:8080/11032
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_local_replica.cc
11 files changed, 256 insertions(+), 239 deletions(-)
Approvals:
Alexey Serbin: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tablet] clean-up on replay of WAL entries
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 )
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log_util.h
File src/kudu/consensus/log_util.h:
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log_util.h@208
PS1, Line 208: Status ReadEntries(std::vector<std::unique_ptr<LogEntryPB>>* entries);
You could use LogEntries here too, if you're willing to define it outside of log-test-base.h.
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/tablet/tablet_bootstrap.cc@473
PS1, Line 473: //string debug_str = entry ? SecureShortDebugString(*entry) : "";
Remove this?
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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-Comment-Date: Tue, 24 Jul 2018 18:15:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] clean-up on replay of WAL entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11032
to look at the new patch set (#2).
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
[tablet] clean-up on replay of WAL entries
Simplified handling of the ownership of log entries read from disk.
This changelist also contains other changes in tablet_bootstrap.cc
and corresponding WAL utilities:
* reduced number of memory allocations while reading WAL entries
* replaced gscoped_ptr with std::unique_ptr
* other minor updates around WAL-related utilities and tests
Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
---
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_local_replica.cc
11 files changed, 256 insertions(+), 239 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/11032/2
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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] [tablet] clean-up on replay of WAL entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 )
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
Patch Set 2: Verified+1
Unrelated flakes in
org.apache.kudu.spark.kudu.DefaultSourceTest.socket read timeout propagation
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Jul 2018 20:02:24 +0000
Gerrit-HasComments: No
[kudu-CR] [tablet] clean-up on replay of WAL entries
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 )
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11032/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11032/1//COMMIT_MSG@9
PS1, Line 9: Reduced number of memory allocations while reading WAL entries from
: disk during tablet bootstrap.
which memory allocation was reduced? I'm having a hard time finding the functional change among the scoped_ptr-related changes.
It seems like if we really wanted to improve performance here we would allow LogEntryPBs to be recycled -- ie instead of freeing an entry, we would pass it back into the LogReader to be reused when reading the next entry. This would keep us from having to continually free and reallocate the relatively large strings that usually end up inside LogEntryPB, and I think provide a measurable boost.
Does this change get us closer to that or farther away? Or make no difference in that respect?
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Jul 2018 15:58:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] clean-up on replay of WAL entries
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 )
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Jul 2018 20:20:31 +0000
Gerrit-HasComments: No
[kudu-CR] [tablet] clean-up on replay of WAL entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 )
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log-test.cc
File src/kudu/consensus/log-test.cc:
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log-test.cc@293
PS1, Line 293: // TODO(unknown): put this in TearDown() with a test on log state?
> warning: missing username/bug in TODO [google-readability-todo]
Done
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log_util.h
File src/kudu/consensus/log_util.h:
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log_util.h@208
PS1, Line 208: // the log entries read up to the corrupted one are returned in the 'entries'
> You could use LogEntries here too, if you're willing to define it outside o
Done
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:
http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/tablet/tablet_bootstrap.cc@473
PS1, Line 473: string debug_str = entry_debug_info;
> Remove this?
Done
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Jul 2018 20:02:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tablet] clean-up on replay of WAL entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11032 )
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tablet] clean-up on replay of WAL entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 )
Change subject: [tablet] clean-up on replay of WAL entries
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11032/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11032/1//COMMIT_MSG@9
PS1, Line 9: Reduced number of memory allocations while reading WAL entries from
: disk during tablet bootstrap.
> which memory allocation was reduced? I'm having a hard time finding the fun
Before the patch there was an extra call to 'new LogEntryPB' in TabletBootstrap::PlaySegments(). Now it's gone.
The purpose of this changelist is not to improve performance, but rather simplify passing LogEntryPB ownership around.
--
To view, visit http://gerrit.cloudera.org:8080/11032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb
Gerrit-Change-Number: 11032
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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-Comment-Date: Tue, 24 Jul 2018 18:13:33 +0000
Gerrit-HasComments: Yes