You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2019/11/19 12:34:06 UTC
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14743
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/trace_metrics.cc
4 files changed, 276 insertions(+), 190 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/1
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/data_dirs.h@225
PS10, Line 225: // Defaults to the value of FLAGS_block_manager.
> This doesn't seem like state that the DataDir should manage, given how tran
Done
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/log_block_manager.cc@2483
PS10, Line 2483: if (!TryStripSuffixString(
> Don't we want to parallelize this? It's at least two seeks: one for the dat
Done
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Thu, 05 Dec 2019 03:58:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#2).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/trace_metrics.cc
4 files changed, 277 insertions(+), 190 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/2
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 15
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#4).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 283 insertions(+), 191 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/4
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 10:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/data_dirs.h@225
PS10, Line 225: std::vector<scoped_refptr<LoadResult>> load_results_;
This doesn't seem like state that the DataDir should manage, given how transient it is (i.e. only used at startup). And constraining it to the LBM ought to remove the need for inheritance in the LoadResult definition too.
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/data_dirs.cc@104
PS10, Line 104: DEFINE_uint64(fs_thread_count_per_data_dir, 5,
Could you run some benchmarks to determine an optimal value here, at least for a spinning disk? SSDs and NVMes should use higher values, but until we we can detect that kind of hardware, we should use the lowest common denominator.
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/10/src/kudu/fs/log_block_manager.cc@2483
PS10, Line 2483: s = LogBlockContainer::Open(this, dir, &result->report, container_name, &container);
Don't we want to parallelize this? It's at least two seeks: one for the data file and one for the metadata file. And I believe LBM startup time is dominated by seeks, not by reads.
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Mon, 02 Dec 2019 19:49:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2411
PS5, Line 2411: "Unable to append deletion record to block metadata");
> Ah, yeah I forgot about that restriction. And it'd be difficult to address.
I'd prefer the later idea, the former one is more difficult to design, and might make other developers confused to use.
I did some benchmarks after the last patch set, it provides similar performance as what I wrote in the commit message.
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2414
PS5, Line 2414: deleted->emplace_back(lb->block_id());
> This is where consolidating behind the DataDir thread pool makes it easier
Done
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Sat, 30 Nov 2019 10:30:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 11:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14743/11//COMMIT_MSG
Commit Message:
PS11:
Please rerun your benchmarks, experimenting with different values for --fs_thread_count_per_data_dir. Possibly with different hardware, if you have access to spinning disks, SSDs, and NVMes.
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@1505
PS11, Line 1505: struct LogBlockContainerLoadResult : public RefCountedThreadSafe<LogBlockContainerLoadResult> {
Why does this need shared ownership?
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@1511
PS11, Line 1511: std::vector<LogBlockContainerRefPtr> dead_containers;
Don't need std:: prefixes here and below.
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@2077
PS11, Line 2077: dir_result->dead_containers.insert(
Why not just std::move()?
dir_result->dead_containers = std::move(container_result->dead_containers);
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@2094
PS11, Line 2094: dir_result
Since this is going to be written to, pass it by (unretained) pointer.
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@2495
PS11, Line 2495: results->push_back(result);
Use emplace_back.
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@2514
PS11, Line 2514: // Open the container asynchronously.
Nit: "Load the container's records asynchronously."
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Thu, 05 Dec 2019 04:45:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure existing concurrent data directories load are effective,
and adjust 'fs_max_thread_count_per_data_dir' and
'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), in milliseconds,
result details as follow:
disk type: SSD
| new version
Block count old version | 1 thread | 2 threads | 4 threads | 8 threads | 16 threads | 32 threads
100,000 2,375 2,382 2,342 2,372 2,343 2,353 2,393
1,000,000 24,018 23,813 22,628 22,407 22,367 22,636 23,173
2,000,000 50,163 51,120 39,726 37,589 37,671 37,501 37,710
4,000,000 104,051 105,560 90,427 79,778 73,129 73,205 74,947
8,000,000 214,347 216,210 199,456 159,143 157,190 158,798 157,056
disk type: spinning disk
| new version
Block count old version | 1 thread | 2 threads | 4 threads | 8 threads | 16 threads | 32 threads
100,000 3,207 3,347 3,345 3,279 3,237 3,263 3,221
1,000,000 33,659 34,106 32,081 30,261 30,142 30,115 30,876
2,000,000 68,097 74,939 56,976 51,407 50,957 56,299 58,456
4,000,000 146,503 162,389 116,956 104,435 94,905 102,606 100,526
8,000,000 331,201 349,609 267,259 247,069 243,064 247,810 247,472
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Reviewed-on: http://gerrit.cloudera.org:8080/14743
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
4 files changed, 339 insertions(+), 250 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 16
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2411
PS5, Line 2411: void LogBlockManager::OpenDataDir(DataDir* dir,
> Yeah making it a gflag seems like a good idea. Eventually we'd want to auto
Thanks for introducing ThreadPoolToken. But I found that we can't Wait a token in the same thread pool we created the token by, that would fail in ThreadPool::CheckNotPoolThreadUnlocked.
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 23 Nov 2019 08:55:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/data_dirs.cc@104
PS13, Line 104: fs_thread_count_per_data_dir
> nit: should this be named as max_fs_thread_count_per_data_dir to reflect it
Good feedback, but let's retain "fs_" as the prefix to keep the grouping with other "fs_" parameters.
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Fri, 06 Dec 2019 22:30:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello wangning, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#13).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'fs_thread_count_per_data_dir' and
'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), in milliseconds,
result details as follow:
disk type: SSD
| new version
Block count old version | 1 thread | 2 threads | 4 threads | 8 threads | 16 threads | 32 threads
100,000 2,375 2,382 2,342 2,372 2,343 2,353 2,393
1,000,000 24,018 23,813 22,628 22,407 22,367 22,636 23,173
2,000,000 50,163 51,120 39,726 37,589 37,671 37,501 37,710
4,000,000 104,051 105,560 90,427 79,778 73,129 73,205 74,947
8,000,000 214,347 216,210 199,456 159,143 157,190 158,798 212,501
disk type: spinning disk
| new version
Block count old version | 1 thread | 2 threads | 4 threads | 8 threads | 16 threads | 32 threads
100,000 3,207 3,347 3,345 3,279 3,237 3,263 3,221
1,000,000 33,659 34,106 32,081 30,261 30,142 30,115 30,876
2,000,000 68,097 74,939 56,976 51,407 50,957 56,299 58,456
4,000,000 146,503 162,389 116,956 104,435 94,905 102,606 100,526
8,000,000 331,201 349,609 267,259 247,069 243,064 247,810 247,472
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
4 files changed, 332 insertions(+), 249 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/13
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 13: Verified+1
(2 comments)
LGTM, just some nits.
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/data_dirs.cc@104
PS13, Line 104: fs_thread_count_per_data_dir
nit: should this be named as max_fs_thread_count_per_data_dir to reflect it is the maximum?
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.h@332
PS13, Line 332: RepairTask
nit: can you add a comment for this method?
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Fri, 06 Dec 2019 21:55:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14743/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14743/13//COMMIT_MSG@29
PS13, Line 29: 157,056
> These higher times are indicating that we're doing IO from too many threads
I reran it several times, found it's not so high. I've updated it.
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/data_dirs.cc@104
PS13, Line 104: fs_max_thread_count_per_data
> Good feedback, but let's retain "fs_" as the prefix to keep the grouping wi
Done
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.h@332
PS13, Line 332: mple wrapp
> nit: can you add a comment for this method?
Done
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 14
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Sat, 07 Dec 2019 03:58:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2411
PS5, Line 2411: void LogBlockManager::OpenDataDir(DataDir* dir,
> - Yes, especially for SSDs and NVMEs. But for HDDs, it's better to be 1, so
Yeah making it a gflag seems like a good idea. Eventually we'd want to autodetect a good value based on the hardware characteristics.
As for the second problem, OpenDataDir can create a ThreadPoolToken on the data dir's thread pool, submit LoadRecords using the token, then Wait on the token rather than the pool, which means it'll wait on just those tasks. See the comments in threadpool.h for more details. Hopefully you can implement that without poking too many holes in the DataDir abstraction.
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2414
PS5, Line 2414: scoped_refptr<internal::ContainerLoadResult> data_dir_result(new internal::ContainerLoadResult());
> I think the best max thread value depends on containers count per data dir,
This is where consolidating behind the DataDir thread pool makes it easier to reason about the situation: the only thing the operator needs to figure out is the max threads of the data dir's threadpool, and it should reflect the number of concurrent operations the underlying hardware can process.
I'd expect LoadRecords to be I/O bound so not sure the number of CPU cores matters.
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 19:52:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello wangning, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#14).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'fs_max_thread_count_per_data_dir' and
'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), in milliseconds,
result details as follow:
disk type: SSD
| new version
Block count old version | 1 thread | 2 threads | 4 threads | 8 threads | 16 threads | 32 threads
100,000 2,375 2,382 2,342 2,372 2,343 2,353 2,393
1,000,000 24,018 23,813 22,628 22,407 22,367 22,636 23,173
2,000,000 50,163 51,120 39,726 37,589 37,671 37,501 37,710
4,000,000 104,051 105,560 90,427 79,778 73,129 73,205 74,947
8,000,000 214,347 216,210 199,456 159,143 157,190 158,798 157,056
disk type: spinning disk
| new version
Block count old version | 1 thread | 2 threads | 4 threads | 8 threads | 16 threads | 32 threads
100,000 3,207 3,347 3,345 3,279 3,237 3,263 3,221
1,000,000 33,659 34,106 32,081 30,261 30,142 30,115 30,876
2,000,000 68,097 74,939 56,976 51,407 50,957 56,299 58,456
4,000,000 146,503 162,389 116,956 104,435 94,905 102,606 100,526
8,000,000 331,201 349,609 267,259 247,069 243,064 247,810 247,472
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
4 files changed, 333 insertions(+), 249 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/14
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 14
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello wangning, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#10).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
8,000,000 214,347 153,435
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 345 insertions(+), 264 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/10
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2411
PS5, Line 2411: record.set_op_type(DELETE);
> Thanks for introducing ThreadPoolToken. But I found that we can't Wait a t
Ah, yeah I forgot about that restriction. And it'd be difficult to address.
We could use a "continuation" design pattern where OpenDataDir doesn't need to wait on the token by moving all of the post-wait work into the last LoadRecords call, but that'll require more state and synchronization.
So let's use your first approach instead, but let's firm up the abstraction somewhat. Let's define two first-class per-data-dir citizen pools:
- One for "control" operations (i.e. operations where throughput isn't a consideration). This is the existing data dir threadpool, with just one thread per pool. It'll be used for temp directory cleanup and for the OpenDataDir fan out.
- One for "data" operations. This will be a new threadpool with a configurable number of max threads (by gflag and perhaps, in the future, by hardware characteristics). Should it be in the DataDir abstraction or should it be in the LBM? I'm not sure, but either way let's use it for hole punching as well as for parallel container loading.
The split is arbitrary and it'll be difficult to decide when to use one pool vs. the other, but the abstraction will hopefully make the code more understandable.
Here's another idea: could we avoid the multi-level parallelism altogether? What if LBM::Open iterated over the data directories and fanned out calls to LoadRecords? Then when all records were loaded, it could fan out calls to Repair (one per data dir). If we could get that to work without too much additional state/synchronization, we could stick to the original idea of expanding the parallelism of the data dir's existing thread pool.
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 23:06:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 13: Code-Review+1
Looks good to me but it'd be nice if one of Hao or Andrew (or both) also took a look.
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Fri, 06 Dec 2019 19:21:50 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14743/12//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14743/12//COMMIT_MSG@32
PS12, Line 32: | new version
> Interesting. Your results suggest that >1 threads on spinning disks are ben
Done
http://gerrit.cloudera.org:8080/#/c/14743/12/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14743/12/src/kudu/fs/log_block_manager.h@261
PS12, Line 261: // Adds an as of yet unseen container to this block manager.
> This is only used in one place now; could we remove it to avoid confusion w
Done
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Fri, 06 Dec 2019 11:36:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager-test.cc@1028
PS5, Line 1028: const int kNumBlocks = AllowSlowTests() ? FLAGS_startup_benchmark_block_count_for_testing : 1000;
Probably cleaner to use OverrideFlagForSlowTests(). Then FLAGS_... is still the right place to go for the value; no need to think about an intermediary kNumBlocks variable.
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@20
PS5, Line 20: #include <errno.h>
Why the reordering? In any case, since you're changing this, could you use <cerrno> instead?
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@37
PS5, Line 37: #include <boost/bind.hpp>
I think you can use std::bind instead (from <functional>).
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2411
PS5, Line 2411: // Create a per-dir thread pool.
Each DataDir has a singleton thread pool, currently used to run OpenDataDir. I think your goal here is to queue more than one operation per disk, since disks (especially SSDs and NVMEs) can run concurrent I/Os, right? If so, why not reuse that thread pool and make it a non-singleton? There could be positive knock-on effects to doing that, like better concurrent hole punching.
You'd still need to do the refactoring you did here, but you should be able to reuse those thread pools.
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2414
PS5, Line 2414: .set_max_threads(5)
You should also experiment with different max thread values. Do you expect "one data dir + many max threads" per disk to be equivalent to "many data dirs + fewer max threads" per disk?
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2417
PS5, Line 2417: vector<shared_ptr<FsReport>> local_report_vec;
These are all sized to "number of containers", right? Could you define a struct to include all of this, and one vector for that struct?
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 20 Nov 2019 20:17:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, wangning, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#15).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure existing concurrent data directories load are effective,
and adjust 'fs_max_thread_count_per_data_dir' and
'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), in milliseconds,
result details as follow:
disk type: SSD
| new version
Block count old version | 1 thread | 2 threads | 4 threads | 8 threads | 16 threads | 32 threads
100,000 2,375 2,382 2,342 2,372 2,343 2,353 2,393
1,000,000 24,018 23,813 22,628 22,407 22,367 22,636 23,173
2,000,000 50,163 51,120 39,726 37,589 37,671 37,501 37,710
4,000,000 104,051 105,560 90,427 79,778 73,129 73,205 74,947
8,000,000 214,347 216,210 199,456 159,143 157,190 158,798 157,056
disk type: spinning disk
| new version
Block count old version | 1 thread | 2 threads | 4 threads | 8 threads | 16 threads | 32 threads
100,000 3,207 3,347 3,345 3,279 3,237 3,263 3,221
1,000,000 33,659 34,106 32,081 30,261 30,142 30,115 30,876
2,000,000 68,097 74,939 56,976 51,407 50,957 56,299 58,456
4,000,000 146,503 162,389 116,956 104,435 94,905 102,606 100,526
8,000,000 331,201 349,609 267,259 247,069 243,064 247,810 247,472
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
4 files changed, 339 insertions(+), 251 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/15
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 15
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 13:
(3 comments)
few nits
http://gerrit.cloudera.org:8080/#/c/14743/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14743/13//COMMIT_MSG@16
PS13, Line 16: exsiting
existing
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.cc@1977
PS13, Line 1977: if (PREDICT_FALSE(!(s).ok())) { \
: if (!(s).IsDiskFailure()) { \
: return s; \
: } \
: LOG(ERROR) << Substitute("Not using report from $0: $1", \
: (d)->dir(), (s).ToString()); \
: }
nit: this macro is naturally used in expressions like
...
RETURN_ON_NON_DISK_FAILURE(dir, s);
...
The trailing semicolon is superfluous, but it would look strange otherwise.
Maybe, use the standard
do {
... the code of the macro goes here ...
} while (false)
pattern for this macro?
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.cc@2065
PS13, Line 2065: if (PREDICT_FALSE(!s.ok())) continue;
style nit: please use scope braces for if (...)
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Sat, 07 Dec 2019 04:37:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#5).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
8,000,000 214,347 153,435
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 283 insertions(+), 191 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/5
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 15: Verified+1 Code-Review+2
Overriding Jenkins, unrelated test failure.
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 15
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 04:15:08 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#6).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
8,000,000 214,347 153,435
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 276 insertions(+), 210 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/6
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 12:
(3 comments)
Hao and Andrew, could you two take a look at this?
http://gerrit.cloudera.org:8080/#/c/14743/12//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14743/12//COMMIT_MSG@32
PS12, Line 32: Block count old version | 1 thread | 5 threads
Interesting. Your results suggest that >1 threads on spinning disks are beneficial, so let's try to figure out what a good default value would be.
Could you rerun with additional powers-of-2 thread values? Let's try 1, 2, 4, 8, 16, 32... until the results get worse.
http://gerrit.cloudera.org:8080/#/c/14743/12/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/14743/12/src/kudu/fs/log_block_manager.h@261
PS12, Line 261: typedef std::unique_ptr<internal::LogBlockContainerLoadResult> LoadResult;
This is only used in one place now; could we remove it to avoid confusion with LogBlockContainerLoadResult?
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@2077
PS11, Line 2077: dir_result->dead_containers.insert(
> Here I aim to _append_ multiple container_results' dead_containers to dir_r
Ah, I missed the inner loop. Makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Thu, 05 Dec 2019 19:04:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#3).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 276 insertions(+), 189 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/3
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 13: Code-Review+2
(1 comment)
Looks good! Just on clarifying question about the results.
http://gerrit.cloudera.org:8080/#/c/14743/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14743/13//COMMIT_MSG@29
PS13, Line 29: 212,501
These higher times are indicating that we're doing IO from too many threads, which is overall slowing down the entire process. Is that right?
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Fri, 06 Dec 2019 20:19:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello wangning, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#12).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'fs_thread_count_per_data_dir' and
'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
disk type: SSD
| new version
Block count old version | 1 thread | 5 threads
100,000 2,375 2,382 2,359
1,000,000 24,018 23,813 22,525
2,000,000 50,163 51,120 37,302
4,000,000 104,051 105,560 74,339
8,000,000 214,347 216,210 154,795
disk type: spinning disk
| new version
Block count old version | 1 thread | 5 threads
100,000 3,207 3,347 3,272
1,000,000 33,659 34,106 32,512
2,000,000 68,097 74,939 55,194
4,000,000 146,503 162,389 112,068
8,000,000 331,201 349,609 258,011
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
4 files changed, 330 insertions(+), 247 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/12
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 12:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14743/11//COMMIT_MSG
Commit Message:
PS11:
> Please rerun your benchmarks, experimenting with different values for --fs_
Run some more benchmarks, but, I don't have machine with NVMes :(
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@1505
PS11, Line 1505: struct LogBlockContainerLoadResult {
> Why does this need shared ownership?
Done
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@1511
PS11, Line 1511: vector<LogBlockContainerRefPtr> dead_containers;
> Don't need std:: prefixes here and below.
Done
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@2077
PS11, Line 2077: dir_result->dead_containers.insert(
> Why not just std::move()?
Here I aim to _append_ multiple container_results' dead_containers to dir_result's, not _assign_.
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@2094
PS11, Line 2094:
> Since this is going to be written to, pass it by (unretained) pointer.
Done
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@2495
PS11, Line 2495: results->emplace_back(new internal::LogBlockContainerLoadResult());
> Use emplace_back.
Done
http://gerrit.cloudera.org:8080/#/c/14743/11/src/kudu/fs/log_block_manager.cc@2514
PS11, Line 2514: // Load the container's records asynchronously.
> Nit: "Load the container's records asynchronously."
Done
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Thu, 05 Dec 2019 12:39:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello wangning, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#8).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
8,000,000 214,347 153,435
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 364 insertions(+), 245 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/8
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 15:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14743/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14743/13//COMMIT_MSG@16
PS13, Line 16: existing
> existing
Done
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.cc@1977
PS13, Line 1977: do { \
: if (PREDICT_FALSE(!(s).ok())) { \
: if (!(s).IsDiskFailure()) { \
: return s; \
: } \
: LOG(ERROR) << Substitute("Not using report from $0: $1", \
:
> nit: this macro is naturally used in expressions like
Done
http://gerrit.cloudera.org:8080/#/c/14743/13/src/kudu/fs/log_block_manager.cc@2065
PS13, Line 2065: RETURN_ON_NON_DISK_FAILURE(dd, s);
> style nit: please use scope braces for if (...)
Done
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 15
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
Gerrit-Comment-Date: Sat, 07 Dec 2019 07:05:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello wangning, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#9).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
8,000,000 214,347 153,435
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 364 insertions(+), 245 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/9
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14743 )
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager-test.cc@1028
PS5, Line 1028: const int kNumBlocks = AllowSlowTests() ? FLAGS_startup_benchmark_block_count_for_testing : 1000;
> Probably cleaner to use OverrideFlagForSlowTests(). Then FLAGS_... is still
Here I'm going to do benchmarks with different large values, OverrideFlagForSlowTests seems to override a FLAGS_xxx by one specified value, it dosen't match.
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@20
PS5, Line 20: #include <algorithm>
> Why the reordering? In any case, since you're changing this, could you use
Done
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@37
PS5, Line 37: #include <gflags/gflags.h>
> I think you can use std::bind instead (from <functional>).
Done
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2411
PS5, Line 2411: FsReport* report,
> Each DataDir has a singleton thread pool, currently used to run OpenDataDir
- Yes, especially for SSDs and NVMEs. But for HDDs, it's better to be 1, so this value should be a gflag?
- If we reuse thread pool of DataDir, when DataDir itself call ExecClosure to excute LogBlockManager::OpenDataDir, then we SubmitFunc by the same thread pool to LogBlockManager::LoadRecords, how do we just wait the later tasks done?
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2414
PS5, Line 2414: data_dir_result.report.data_dirs.push_back(dir->dir());
> You should also experiment with different max thread values. Do you expect
I think the best max thread value depends on containers count per data dir, total data dirs count and CPU cores, a gflag may be better?
Yes, I expect they are equal. We can specify multiple data dirs on a single disk, right? I didn't try that ever. For historical reason, some users may specify only one data dir on a disk, "one data dir + many max threads" may improve startup time.
http://gerrit.cloudera.org:8080/#/c/14743/5/src/kudu/fs/log_block_manager.cc@2417
PS5, Line 2417: unordered_set<string> containers_seen;
> These are all sized to "number of containers", right? Could you define a st
Done
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 08:39:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#7).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
8,000,000 214,347 153,435
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 277 insertions(+), 210 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/7
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-3001 Multi-thread to load containers in a data directory
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello wangning, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14743
to look at the new patch set (#11).
Change subject: KUDU-3001 Multi-thread to load containers in a data directory
......................................................................
KUDU-3001 Multi-thread to load containers in a data directory
When a data directory has many block containers, a single thread to
load these container files is low efficiency, we can improve it by
multi-threads.
We did some simple benchmarks to verify it. Adjust
'log_container_max_size' to 1GB to generate more containers when do
benchmarks, adjust 'startup_benchmark_data_dir_count_for_testing' to 8
to make sure exsiting concurrent data directories load are effective,
and adjust 'startup_benchmark_block_count_for_testing' to different
values, timing 10 times ReopenBlockManager(), result details as follow:
Block count single thread multi-threads
100,000 2,375 2,320
1,000,000 24,018 22,564
2,000,000 50,163 37,447
4,000,000 104,051 74,666
8,000,000 214,347 153,435
Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
4 files changed, 330 insertions(+), 247 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14743/11
--
To view, visit http://gerrit.cloudera.org:8080/14743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0721ee4a5a6824db146ba0658e60eec25dd0c65c
Gerrit-Change-Number: 14743
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: wangning <19...@gmail.com>