Adar Dembo has posted comments on this change.

Change subject: [tests] fix test which fails with two cpus and document other 

Patch Set 6:

Commit Message:

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 
of CPUs,
            : 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
Nit: shards
File docs/installation.adoc:

Please update the various script examples in this file, as well as the SLES 
File src/kudu/codegen/

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)?
File src/kudu/gutil/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I81b70f63923078d449f6541a61b292517e49877d
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Brock Noland <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-Reviewer: Will Berkeley <>
Gerrit-HasComments: Yes

Reply via email to