Yingchun Lai 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 4: (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 Done 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: > Since the test fixture is stateless, there's no value gained by defining Te Done http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc@88 PS1, Line 88: for (int i = 0; i < kMaterialCount; ++i) { > Heap allocate this so that the thread stack isn't blown out if someone cust Done http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc@89 PS1, Line 89: materials.emplace_back(RandomString(kInputSize, &random)); > This suggests that perhaps the benchmark should be structured as a single t Done http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc@98 PS1, Line 98: > Random::Uniform() is more clear for this. Done http://gerrit.cloudera.org:8080/#/c/14086/1/src/kudu/util/compression/compression-test.cc@123 PS1, Line 123: } > Here and in Execute Uncompress consider using LOG_TIMING(), a useful macro I saw implemention of LOG_TIMING, but the internal stopwatch couldn't be exposed, so we can't calculate thoughput if not declare another stopwatch outside. 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: } // namespace kudu : #endif > Could you get by with CompressionType_Name? It's auto-generated by protoc: Done -- 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: 4 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Mon, 19 Aug 2019 15:29:22 +0000 Gerrit-HasComments: Yes
