Adar Dembo has posted comments on this change.
Change subject: [tests] fix test which fails with two cpus and document other
Patch Set 6:
PS6, Line 16: SharedLRUCache
Nit: this should be ShardedLRUCache. Below too.
PS6, Line 17: 2) the fact that capacity of SharedLRUCache is higher than the
: configured if the configured capacity is not divisible by the the
: of CPUs.
: For example, the capacity is set here:
: FLAGS_codegen_cache_capacity = 10;
: However, if the capacity is not perfectly divisible by the number
: actual capacity is slightly higher.
: CPU 2 => Capacity 10, 5/shard
: CPU 4 => Capacity 12, 3/shard
: CPU 8 => Capacity 16, 2/shard
: Due to this calculation:
: const size_t per_shard = (capacity + (num_shards - 1)) /
I appreciate the detailed explanation here, but I'm having a hard time
understanding it without looking at the code too. Maybe you can work in how
num_shards is calculated? And make explicit that the actual capacity is
per_shard * num_shards?
Also, in each of the examples you've provided, the capacity is perfectly
divisible by the number of CPUs. Maybe add some negative examples?
PS6, Line 47: shard's
Please update the various script examples in this file, as well as the SLES
Line 373: FLAGS_codegen_cache_capacity = 20;
This fixes the test for two CPUs, but can we either fix the test to be more
robust when the capacity isn't exactly 20, or fix the underlying sharding
implementation to always use the provided capacity (maybe we'd need to enforce
only certain kinds of capacity values e.g. powers of 2 to do this)?
PS6, Line 64: "Advanced option. Use at your own risk");
I think what Todd meant by "flag it as advanced" was that you use TAG_FLAG to
explicitly tag it.
However, tag_flags is part of the util module, which depends on gutil, so doing
so would yield a circular dependency. One way to address it would be as follows:
1) Add a kudu::NumCPUs() method somewhere in the util module.
2) Make it call base::NumCPUs().
3) Define this new flag and handle the override there.
4) Change all existing callers to use kudu::NumCPUs() instead.
To view, visit http://gerrit.cloudera.org:8080/4446
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Brock Noland <br...@phdata.io>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>