Adar Dembo has posted comments on this change. Change subject: [tests] fix test which fails with two cpus and document other dependencies ......................................................................
Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/4446/6//COMMIT_MSG 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 capacity : configured if the configured capacity is not divisible by the the number : 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)) / num_shards; 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 http://gerrit.cloudera.org:8080/#/c/4446/6/docs/installation.adoc File docs/installation.adoc: Please update the various script examples in this file, as well as the SLES instructions. http://gerrit.cloudera.org:8080/#/c/4446/6/src/kudu/codegen/codegen-test.cc File src/kudu/codegen/codegen-test.cc: 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)? http://gerrit.cloudera.org:8080/#/c/4446/6/src/kudu/gutil/sysinfo.cc File src/kudu/gutil/sysinfo.cc: 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-MessageType: comment Gerrit-Change-Id: I81b70f63923078d449f6541a61b292517e49877d Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master 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> Gerrit-HasComments: Yes
