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 2020/05/21 00:10:46 UTC
[kudu-CR] WIP [master] cache for table locations
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15971
Change subject: WIP [master] cache for table locations
......................................................................
WIP [master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
With the cache enabled, the performance of the
CatalogManager::GetTableLocations method improved about 1.7x times
and a small improvement for GetTableLocations RPC according to
GetTableLocationsBenchmarkFunctionCall scenario from
the TableLocationsTest suite:
with cache disabled:
GetTableLocations function call: 157894.6 req/sec
GetTableLocations RPC: 14749 req/sec
with cache enabled:
GetTableLocations function call: 268782.2 req/sec
GetTableLocations RPC: 16911.4 req/sec
WIP:
* the only test failing is ClientTest.TestPartitione; need to clarify
on the reason
* parameterize a few tests to run it with and without
table location cache
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
8 files changed, 502 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/1
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [master] cache for table locations
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc@145
PS2, Line 145: DCHECK((ptr_ != nullptr && handle_) ||
> I'll keep it here for debugging as other VLOG() messages.
Oops, seems gerrit didn't highlight properly. You can drop the 'strings::', I meant.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jun 2020 05:26:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: WIP [master] cache for table locations
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/1//COMMIT_MSG
Commit Message:
PS1:
I also did some integration-level testing with a synthetic workload against a real Kudu cluster. It turned out that the performance improvement is negligible when turning on the cache for tablet locations.
Maybe, there are some specific partition schemas that could benefit more, but at this point I'm not sure it's worth caching the locations, at least as it's done in this patch.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 21 May 2020 21:51:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/integration-tests/table_locations-itest.cc@455
PS2, Line 455: if (!proxies[i]->GetTableLocations(req, &resp, &controller).ok()) {
> nit: while you're here, may be nice to log the error
Done
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@317
PS2, Line 317: MiBytes
> nit: mebibytes or MiB?
Done
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@5130
PS2, Line 5130: table_locations_cache_
> Hmm.. Do you think it's worth predicating on FLAGS_table_locations_cache_ca
Not sure this is relevant: this code is unreachable if master is not a leader, IIUC.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@5178
PS2, Line 5178: info
> nit: into
Done
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.h@125
PS2, Line 125: EvictedEntry
> nit: maybe call this Run() or somesuch? Also could you mention what 'key' a
I don't want to change method names in this interface, at least in this changelist. Maybe, sometime in a separate changelist?
I added a comment to describe the parameters.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc@145
PS2, Line 145: VLOG(2) << strings::Substitute("EvictedEntry callback for key '$0'",
> nit: can drop
I'll keep it here for debugging as other VLOG() messages.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jun 2020 05:19:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 10 Jul 2020 06:54:52 +0000
Gerrit-HasComments: No
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Greg Solovyev, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#8).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 130%
when the cache is enabled.
A smaller 12% improvement is observed for GetTableLocations RPC
when running with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 588056.2 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 39328.6 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 196643
Mean: 149.526
Percentiles:
0% (min) = 40
25% = 120
50% (med) = 142
75% = 172
95% = 216
99% = 308
99.9% = 652
99.99% = 1424
100% (max) = 8480
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48
GetTableLocations RPC: 34833.2 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174166
Mean: 241.475
Percentiles:
0% (min) = 61
25% = 189
50% (med) = 225
75% = 278
95% = 374
99% = 532
99.9% = 956
99.99% = 1848
100% (max) = 5834
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 1,001 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/8
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15971/3//COMMIT_MSG@14
PS3, Line 14: A smaller 12% improvement is observed for GetTableLocations RPC
Not to be too much of a downer, but is this complexity worth it for 12% end-to-end? As I understand it, the 12% is for the case when all the clients are sending the exact same request, which may not be the case in real scenarios, right? (eg each task is likely to be asking for the locations of a different partition in the case of a query)
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jun 2020 05:18:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc@145
PS2, Line 145: VLOG(2) << strings::Substitute("EvictedEntry callback for key '$0'",
> Oops, seems gerrit didn't highlight properly. You can drop the 'strings::',
Done
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jun 2020 05:30:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 100%
when the cache is enabled. Also, 14% improvement is observed for
GetTableLocations RPC when running with the same number of concurrent
client threads and cache enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 504187.6 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 40033.4 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 200167
Mean: 155.144
Percentiles:
0% (min) = 45
25% = 125
50% (med) = 147
75% = 177
95% = 223
99% = 314
99.9% = 652
99.99% = 1928
100% (max) = 5096
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48
GetTableLocations RPC: 34981 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174905
Mean: 249.308
Percentiles:
0% (min) = 64
25% = 197
50% (med) = 231
75% = 284
95% = 386
99% = 556
99.9% = 980
99.99% = 2400
100% (max) = 6113
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Reviewed-on: http://gerrit.cloudera.org:8080/15971
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 1,098 insertions(+), 35 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 13
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 7: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/7/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15971/7/src/kudu/integration-tests/table_locations-itest.cc@805
PS7, Line 805: // different entries in the table locations cache.
nit: this test also verifies that multiple requests with the same essential parameters do not produce multiple different entries - add this to the comment?
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jun 2020 18:28:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#7).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 130%
when the cache is enabled.
A smaller 12% improvement is observed for GetTableLocations RPC
when running with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 588056.2 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 39328.6 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 196643
Mean: 149.526
Percentiles:
0% (min) = 40
25% = 120
50% (med) = 142
75% = 172
95% = 216
99% = 308
99.9% = 652
99.99% = 1424
100% (max) = 8480
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48
GetTableLocations RPC: 34833.2 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174166
Mean: 241.475
Percentiles:
0% (min) = 61
25% = 189
50% (med) = 225
75% = 278
95% = 374
99% = 532
99.9% = 956
99.99% = 1848
100% (max) = 5834
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 941 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/7
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Greg Solovyev, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#10).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 100%
when the cache is enabled.
A smaller 14% improvement is observed for GetTableLocations RPC
when running with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 504187.6 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 40033.4 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 200167
Mean: 155.144
Percentiles:
0% (min) = 45
25% = 125
50% (med) = 147
75% = 177
95% = 223
99% = 314
99.9% = 652
99.99% = 1928
100% (max) = 5096
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48
GetTableLocations RPC: 34981 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174905
Mean: 249.308
Percentiles:
0% (min) = 64
25% = 197
50% (med) = 231
75% = 284
95% = 386
99% = 556
99.9% = 980
99.99% = 2400
100% (max) = 6113
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 1,113 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/10
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/integration-tests/table_locations-itest.cc@445
PS2, Line 445: vector<uint64_t> err_counters(kNumThreads, 0);
> What's the reason for adding error counters? Could you mention about it in
IIUC it's just extra checking to more gracefully fail in case anything goes wrong with these calls -- mostly cosmetic
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/integration-tests/table_locations-itest.cc@455
PS2, Line 455: if (!proxies[i]->GetTableLocations(req, &resp, &controller).ok()) {
nit: while you're here, may be nice to log the error
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@317
PS2, Line 317: MiBytes
nit: mebibytes or MiB?
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@5130
PS2, Line 5130: table_locations_cache_
Hmm.. Do you think it's worth predicating on FLAGS_table_locations_cache_capacity_mb == 0, so we don't have to wait for a leadership transfer to stop caching, perhaps as a failsafe in case for whatever reason we want to stop using cached values?
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@5178
PS2, Line 5178: info
nit: into
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.h@125
PS2, Line 125: EvictedEntry
nit: maybe call this Run() or somesuch? Also could you mention what 'key' and 'val' are here, and what this method is supposed to do with them?
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc@145
PS2, Line 145: VLOG(2) << strings::Substitute("EvictedEntry callback for key '$0'",
nit: can drop
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 20 Jun 2020 02:56:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/integration-tests/table_locations-itest.cc@445
PS2, Line 445: vector<uint64_t> err_counters(kNumThreads, 0);
> What's the reason for adding error counters? Could you mention about it in
Below in the code you can see that the test is declared failed when at least one per-thread error counter is not zero.
Otherwise, it's not easy to bail out cleanly out of the test of calling ASSERT_XXX in one of the threads.
I added a short comment in the code for 'err_counters'.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@1191
PS2, Line 1191: inforamtion
> typo
Done
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.h@80
PS2, Line 80: // This class is not intended to be inherited from.
> Would be better to mark the class as 'final' then.
Done
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc@61
PS2, Line 61: auto h(cache_->Lookup(key, Cache::EXPECT_IN_CACHE));
> I was expecting Get() will go through the 'keys_by_table_id_' map, but look
'keys_by_table_id_' is needed only when invalidating entries in the cache given table identifier.
I added a comment into the header file where 'keys_by_table_id_' is declared.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc@100
PS2, Line 100: cache_->Erase(key);
> I was thinking what happens if the underlying cache decides to evict the en
Erase() doesn't return a status code or an error. By current cache's API, it doesn't matter whether the to-be-erased element was in cache or not, the important fact is that the element will not be in the cache after Erase() call (unless it was inserted right after that, of course).
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache_metrics.cc
File src/kudu/master/table_locations_cache_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache_metrics.cc@63
PS2, Line 63: MINIT(cache_hits_caching, table_locations_cache_hits);
: MINIT(cache_misses_caching, table_locations_cache_misses);
> Why not simply hits and misses like other members?
Because cache_hits_caching and cache_hits are of different semantics in the underlying LRU cache; see https://github.com/apache/kudu/blob/0dc3e9e0a7306ce9b618158b8a20af9b10f4a482/src/kudu/util/cache.cc#L586 and https://github.com/apache/kudu/blob/0dc3e9e0a7306ce9b618158b8a20af9b10f4a482/src/kudu/util/cache.cc#L345
Here we are interested in cache misses when expecting the element in the cache as per https://gerrit.cloudera.org/#/c/15971/2/src/kudu/master/table_locations_cache.cc@61
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 30 Jun 2020 04:57:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 11: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@145
PS10, Line 145: std::unique_ptr<Cache::EvictionCallback> eviction_cb_;
:
: // The underlying LRU cache instance.
: std::unique_ptr<Cache> cache_;
> I guess it might be such a thing, but there is an issue with the signature
Right, isn't that still possible if this were just Cache::EvictionCallback eviction_cb_ and using &eviction_cb_, instead of a unique_ptr and ::get()?
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 09 Jul 2020 22:58:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@5130
PS2, Line 5130: e->GetTabletsInRange(r
> Not sure this is relevant: this code is unreachable if master is not a lead
Right, my point is that if we want to _stop_ using cached locations, we would need to wait for a leadership transfer for table_locations_cache_ to reset, albeit requests would then go to a different master.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jun 2020 06:14:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@145
PS10, Line 145: // Invoked whenever a cached entry reaches zero reference count, i.e. it was
: // removed from the cache and there aren't any other references
: // floating around.
: std::unique_ptr<Cache::EvictionCallback> eviction_cb_;
> Hmm I feel like Im still missing something. At least this seems to work? ht
Ah, OK: even now we use 'this' in the initializer's list for TableLocationsCache and it works for our case. I guess that's fine then. There would be an issue with using 'this' in the initializer's list if those did something like calling virtual functions of the object being constructed: that's what I referred to. But EvictionCallback just stores the pointer in its member field, and that's fine.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 10 Jul 2020 03:16:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@145
PS10, Line 145: // Invoked whenever a cached entry reaches zero reference count, i.e. it was
: // removed from the cache and there aren't any other references
: // floating around.
: std::unique_ptr<Cache::EvictionCallback> eviction_cb_;
> Right, isn't that still possible if this were just Cache::EvictionCallback
Right, it would be possible if creating an member of EvictionCallback without pointer to the class instance were possible. But given the current signature, it's not.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 10 Jul 2020 01:21:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 10:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1081
PS10, Line 1081: void TearDown() override {
: if (cluster_) {
: cluster_->Shutdown();
: cluster_.reset();
: }
: KuduTest::TearDown();
: }
nit: doesn't this happen anyway via cluster_'s destructor?
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1149
PS10, Line 1149: NO_FATALS(CheckCacheMetricsResetAllMasters());
nit: Since this is only used once, perhaps inline it?
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@54
PS10, Line 54: // Copying of entry handles is explicitly prohibited.
: EntryHandle(const EntryHandle&) = delete;
: EntryHandle& operator=(const EntryHandle&) = delete;
Is this different than DISALLOW_COPY_AND_ASSIGN(EntryHandle)?
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@145
PS10, Line 145: // Invoked whenever a cached entry reaches zero reference count, i.e. it was
: // removed from the cache and there aren't any other references
: // floating around.
: std::unique_ptr<Cache::EvictionCallback> eviction_cb_;
Can this be stack-allocated and const?
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@42
PS10, Line 42:
nit: a couple too many spaces
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@95
PS10, Line 95: auto h(cache_->Insert(std::move(pending), eviction_cb_.get()));
If this isn't being done under keys_by_table_id_lock_, isn't it possible that we Put() and immediately Remove(), but end up with the entry added into the cache, and nothing in keys_by_table_id_? I think that'd mean we're leaving memory on the table in the cache since we may never evict those handles.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 08 Jul 2020 06:31:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Greg Solovyev, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#9).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 100%
when the cache is enabled.
A smaller 14% improvement is observed for GetTableLocations RPC
when running with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 504187.6 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 40033.4 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 200167
Mean: 155.144
Percentiles:
0% (min) = 45
25% = 125
50% (med) = 147
75% = 177
95% = 223
99% = 314
99.9% = 652
99.99% = 1928
100% (max) = 5096
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48
GetTableLocations RPC: 34981 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174905
Mean: 249.308
Percentiles:
0% (min) = 64
25% = 197
50% (med) = 231
75% = 284
95% = 386
99% = 556
99.9% = 980
99.99% = 2400
100% (max) = 6113
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 955 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/9
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#5).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 130%
when the cache is enabled.
A smaller 12% improvement is observed for GetTableLocations RPC
when running with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 588056.2 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 39328.6 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 196643
Mean: 149.526
Percentiles:
0% (min) = 40
25% = 120
50% (med) = 142
75% = 172
95% = 216
99% = 308
99.9% = 652
99.99% = 1424
100% (max) = 8480
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48
GetTableLocations RPC: 34833.2 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174166
Mean: 241.475
Percentiles:
0% (min) = 61
25% = 189
50% (med) = 225
75% = 278
95% = 374
99% = 532
99.9% = 956
99.99% = 1848
100% (max) = 5834
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 941 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/5
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#3).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 130%
when the cache is enabled.
A smaller 12% improvement is observed for GetTableLocations RPC
when running with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 588056.2 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 39328.6 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 196643
Mean: 149.526
Percentiles:
0% (min) = 40
25% = 120
50% (med) = 142
75% = 172
95% = 216
99% = 308
99.9% = 652
99.99% = 1424
100% (max) = 8480
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48
GetTableLocations RPC: 34833.2 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174166
Mean: 241.475
Percentiles:
0% (min) = 61
25% = 189
50% (med) = 225
75% = 278
95% = 374
99% = 532
99.9% = 956
99.99% = 1848
100% (max) = 5834
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 938 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/3
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15971/3//COMMIT_MSG@14
PS3, Line 14: A smaller 12% improvement is observed for GetTableLocations RPC
> Not to be too much of a downer, but is this complexity worth it for 12% end
Yes, it's a good question. I guess the performance boost will be higher if also optimizing the part where RPC response is copied from an entry in the cache just to be serialized on the wire. In other words, I think that with other improvements in RPC it will get higher.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jun 2020 05:26:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#2).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 130%
when the cache is enabled.
A smaller 12% improvement is observed for GetTableLocations RPC
when run with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 588056.2 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 39328.6 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 196643
Mean: 149.526
Percentiles:
0% (min) = 40
25% = 120
50% (med) = 142
75% = 172
95% = 216
99% = 308
99.9% = 652
99.99% = 1424
100% (max) = 8480
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48
GetTableLocations RPC: 34833.2 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174166
Mean: 241.475
Percentiles:
0% (min) = 61
25% = 189
50% (med) = 225
75% = 278
95% = 374
99% = 532
99.9% = 956
99.99% = 1848
100% (max) = 5834
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 529 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/2
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 10:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1081
PS10, Line 1081: void TearDown() override {
: if (cluster_) {
: cluster_->Shutdown();
: cluster_.reset();
: }
: KuduTest::TearDown();
: }
> nit: doesn't this happen anyway via cluster_'s destructor?
Yes, I think it does. I think this piece is a copy-paste byproduct :)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1149
PS10, Line 1149: NO_FATALS(CheckCacheMetricsResetAllMasters());
> nit: Since this is only used once, perhaps inline it?
Done
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@54
PS10, Line 54: // Copying of entry handles is explicitly prohibited.
: EntryHandle(const EntryHandle&) = delete;
: EntryHandle& operator=(const EntryHandle&) = delete;
> Is this different than DISALLOW_COPY_AND_ASSIGN(EntryHandle)?
I guess it's supposed to be the same, but DISALLOW_COPY_AND_ASSIGN() for some reason has operator=() as void.
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@145
PS10, Line 145: // Invoked whenever a cached entry reaches zero reference count, i.e. it was
: // removed from the cache and there aren't any other references
: // floating around.
: std::unique_ptr<Cache::EvictionCallback> eviction_cb_;
> Can this be stack-allocated and const?
I guess it might be such a thing, but there is an issue with the signature of the EvictionCallback class: it requires a pointer to the outer cache class.
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@42
PS10, Line 42:
> nit: a couple too many spaces
Done
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@95
PS10, Line 95: auto h(cache_->Insert(std::move(pending), eviction_cb_.get()));
> If this isn't being done under keys_by_table_id_lock_, isn't it possible th
Good catch! Fixed.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 09 Jul 2020 01:21:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Greg Solovyev, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#11).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 100%
when the cache is enabled.
A smaller 14% improvement is observed for GetTableLocations RPC
when running with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 504187.6 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 40033.4 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 200167
Mean: 155.144
Percentiles:
0% (min) = 45
25% = 125
50% (med) = 147
75% = 177
95% = 223
99% = 314
99.9% = 652
99.99% = 1928
100% (max) = 5096
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48
GetTableLocations RPC: 34981 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174905
Mean: 249.308
Percentiles:
0% (min) = 64
25% = 197
50% (med) = 231
75% = 284
95% = 386
99% = 556
99.9% = 980
99.99% = 2400
100% (max) = 6113
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 1,098 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/11
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Greg Solovyev, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#12).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 100%
when the cache is enabled. Also, 14% improvement is observed for
GetTableLocations RPC when running with the same number of concurrent
client threads and cache enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 504187.6 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 40033.4 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 200167
Mean: 155.144
Percentiles:
0% (min) = 45
25% = 125
50% (med) = 147
75% = 177
95% = 223
99% = 314
99.9% = 652
99.99% = 1928
100% (max) = 5096
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=32 \
--benchmark_num_threads=48
GetTableLocations RPC: 34981 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174905
Mean: 249.308
Percentiles:
0% (min) = 64
25% = 197
50% (med) = 231
75% = 284
95% = 386
99% = 556
99.9% = 980
99.99% = 2400
100% (max) = 6113
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 1,098 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/12
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 2:
(6 comments)
I mostly focussed on TableLocationsCache class and not call sites.
Is it worth adding a unit test for TableLocationsClass.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/integration-tests/table_locations-itest.cc@445
PS2, Line 445: vector<uint64_t> err_counters(kNumThreads, 0);
What's the reason for adding error counters? Could you mention about it in the commit message.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@1191
PS2, Line 1191: inforamtion
typo
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.h@80
PS2, Line 80: // This class is not intended to be inherited from.
Would be better to mark the class as 'final' then.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc@61
PS2, Line 61: auto h(cache_->Lookup(key, Cache::EXPECT_IN_CACHE));
I was expecting Get() will go through the 'keys_by_table_id_' map, but looks like that's not needed.
Might be worth adding a comment in the header above the map, how it's used.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache.cc@100
PS2, Line 100: cache_->Erase(key);
I was thinking what happens if the underlying cache decides to evict the entry then Erase() call will return an error or such.
But it's okay to erase a non-existent entry.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache_metrics.cc
File src/kudu/master/table_locations_cache_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/table_locations_cache_metrics.cc@63
PS2, Line 63: MINIT(cache_hits_caching, table_locations_cache_hits);
: MINIT(cache_misses_caching, table_locations_cache_misses);
Why not simply hits and misses like other members?
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 17 Jun 2020 18:05:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#4).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 130%
when the cache is enabled.
A smaller 12% improvement is observed for GetTableLocations RPC
when running with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 588056.2 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 39328.6 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 196643
Mean: 149.526
Percentiles:
0% (min) = 40
25% = 120
50% (med) = 142
75% = 172
95% = 216
99% = 308
99.9% = 652
99.99% = 1424
100% (max) = 8480
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48
GetTableLocations RPC: 34833.2 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174166
Mean: 241.475
Percentiles:
0% (min) = 61
25% = 189
50% (med) = 225
75% = 278
95% = 374
99% = 532
99.9% = 956
99.99% = 1848
100% (max) = 5834
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 942 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/4
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 7:
(1 comment)
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/21904/ : FAILURE
It seems the tests need re-work.
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/15971/2/src/kudu/master/catalog_manager.cc@5130
PS2, Line 5130: e->GetTabletsInRange(r
> Hmm.. Do you think it's worth predicating on FLAGS_table_locations_cache_ca
Not sure how it's relevant: if master is not a leader, this
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jun 2020 17:37:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] cache for table locations
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15971
to look at the new patch set (#6).
Change subject: [master] cache for table locations
......................................................................
[master] cache for table locations
This patch introduces a cache for table locations in catalog manager.
When running with 48 concurrent client threads, the performance of
CatalogManager::GetTableLocations() method improved about 130%
when the cache is enabled.
A smaller 12% improvement is observed for GetTableLocations RPC
when running with the same number of concurrent client threads and cache
enabled. The test results are below.
I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh
in a follow up patch.
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations function call: 588056.2 req/sec
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \
--benchmark_num_threads=48
GetTableLocations function call: 252443.4 req/sec
========================================================================
After this patch with 128MByte cache enabled:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48 \
--table_locations_cache_capacity_mb=128
GetTableLocations RPC: 39328.6 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 196643
Mean: 149.526
Percentiles:
0% (min) = 40
25% = 120
50% (med) = 142
75% = 172
95% = 216
99% = 308
99.9% = 652
99.99% = 1424
100% (max) = 8480
Before this patch:
table_locations-itest \
--gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \
--rpc_num_service_threads=24 \
--benchmark_num_threads=48
GetTableLocations RPC: 34833.2 req/sec
Stats on GetTableLocations RPC (times in microseconds):
Count: 174166
Mean: 241.475
Percentiles:
0% (min) = 61
25% = 189
50% (med) = 225
75% = 278
95% = 374
99% = 532
99.9% = 956
99.99% = 1848
100% (max) = 5834
Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
---
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
A src/kudu/master/table_locations_cache.cc
A src/kudu/master/table_locations_cache.h
A src/kudu/master/table_locations_cache_metrics.cc
A src/kudu/master/table_locations_cache_metrics.h
9 files changed, 941 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/6
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [master] cache for table locations
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@145
PS10, Line 145: std::unique_ptr<Cache::EvictionCallback> eviction_cb_;
:
: // The underlying LRU cache instance.
: std::unique_ptr<Cache> cache_;
> Right, it would be possible if creating an member of EvictionCallback witho
Hmm I feel like Im still missing something. At least this seems to work? https://gist.github.com/andrwng/c2f1b8066d4c50fded9f8bb2287e7895
Though making it const wouldn't work, since we can't cast a const Derived* to a Base*
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 10 Jul 2020 01:50:00 +0000
Gerrit-HasComments: Yes