Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14086 )
Change subject: [compression] Refactor unit tests and add simple benchmark test ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/14086/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14086/1//COMMIT_MSG@12 PS1, Line 12: influence Nit: influences http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc File src/kudu/util/compression/compression-test.cc: http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc@45 PS1, Line 45: void TestCompressionCodec() { Since the test fixture is stateless, there's no value gained by defining TestCompressionCodec and Benchmark as fixture methods vs. just putting the logic directly in the tests themselves. http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc@88 PS1, Line 88: char materials[kMaterialCount][kInputSize]; Heap allocate this so that the thread stack isn't blown out if someone customizes kMaterialCount or kInputSize. http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc@89 PS1, Line 89: Random r(0); // Use seed '0' means generate the same data for all compression algorithms. This suggests that perhaps the benchmark should be structured as a single test with a loop over the different compression types. That way you could use SeedRandom() here and actually get useful variety amongst the different invocations of the benchmark. http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc@98 PS1, Line 98: r.Next() % kMaterialCount Random::Uniform() is more clear for this. http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc@123 PS1, Line 123: LOG(INFO) << GetCompressionCodecName(compression) << " compress throughput: " Here and in Execute Uncompress consider using LOG_TIMING(), a useful macro that handles the stopwatch stuff for you. http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression_codec.h File src/kudu/util/compression/compression_codec.h: http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression_codec.h@77 PS1, Line 77: // Returns the compression codec name given the type : std::string GetCompressionCodecName(CompressionType type); Could you get by with CompressionType_Name? It's auto-generated by protoc: https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#enum -- To view, visit http://gerrit.cloudera.org:8080/14086 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If09817d223b98c825d0c8276c8f663b5c5b9eb12 Gerrit-Change-Number: 14086 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 19 Aug 2019 04:20:29 +0000 Gerrit-HasComments: Yes
