Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13425 )
Change subject: IMPALA-8542. Add an access trace for the data cache ...................................................................... Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache-test.cc File be/src/runtime/io/data-cache-test.cc: http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache-test.cc@504 PS2, Line 504: expect_misses); > nit: indent 4 spaces here and below see above http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.h File be/src/runtime/io/data-cache.h: http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.h@269 PS2, Line 269: /// The file name used for the access trace. > nit: newline before this one Done http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc File be/src/runtime/io/data-cache.cc: http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@88 PS2, Line 88: c > nit: Capitalize first word Done http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@97 PS2, Line 97: const char* DataCache::Partition::TRACE_FILE_NAME = "impala-cache-trace.txt"; > Should we include the partition name in the trace to make it easier to copy I think concatenation should be fine, or if we ask users to collect them, we would provide a script that can do the right thing (eg using tar to preserve the original paths). http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@159 PS2, Line 159: explicit Tracer(string path) > nit: single line Done http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@165 PS2, Line 165: logger_->Stop(); > nit: single line Done http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@413 PS2, Line 413: return UNALIGNED_LOAD64(key_.data() + key_.size() - sizeof(int64_t) * 2); > Can you add a comment to point out that these hard-code the key layout? Sho Rearranged the key and added some constants to make this clearer. I didn't end up adding the extra name_len field since it seemed a bit wasteful of memory and not much gain. http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@544 PS2, Line 544: if (tracer_) tracer_->Trace(Tracer::MISS, cache_key, bytes_to_read, /*entry_len=*/-1); > nit: the rest of the file uses explicit pointer comparison (see above), sho Done http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@656 PS2, Line 656: if (tracer_) tracer_->Trace(Tracer::STORE_FAILED_BUSY, cache_key, > style: Enclose multiple lines in {}, indent 4 spaces Enclosed in {} but the style guide says to align the parameters on the second line with the parameters on the first line: https://google.github.io/styleguide/cppguide.html#Function_Calls : bool result = DoSomething(averyveryveryverylongargument1, argument2, argument3); http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@860 PS2, Line 860: S! > Would it make automatic parsing easier if this was a single character, e.g. Done http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@871 PS2, Line 871: C > Can this be a static assert? I don't think CalcualteBase64EscapedLen is constexpr (it's defined in the .cc, not the header) http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@874 PS2, Line 874: sizeof(hash), buf, buf_len); > nit: indent 4 spaces see above, google style guide disagrees http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@903 PS2, Line 903: > nit: extra empty line Done -- To view, visit http://gerrit.cloudera.org:8080/13425 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2302c19abb5db19f1d3d1cd727a82977a9e2ba9c Gerrit-Change-Number: 13425 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 03 Jun 2019 18:33:58 +0000 Gerrit-HasComments: Yes
