Lars Volker 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: (15 comments) Had a question about the naming of the trace file and otherwise just a bunch of nits. 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@492 PS2, Line 492: SCOPED_TRACE TIL about SCOPED_TRACE :) 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 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 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 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 them to a common directory, i.e. when we ask users to collect them? Or do you envision that they concatenate them instead? 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 http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@165 PS2, Line 165: logger_->Stop(); nit: single line 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? Should we switch to an aligned struct (mtime, offset, name_len, filename) instead? 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), should we keep it consistent? 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 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. F? http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@870 PS2, Line 870: buf_len We usually use all-caps for constants 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? nit: missing space 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 http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@903 PS2, Line 903: nit: extra empty line -- 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-Comment-Date: Fri, 31 May 2019 02:37:33 +0000 Gerrit-HasComments: Yes
