[Impala-ASF-CR] IMPALA-8542. Add an access trace for the data cache

["Todd Lipcon (Code Review)" <ge...@cloudera.org>]

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 <to...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 03 Jun 2019 18:33:58 +0000
Gerrit-HasComments: Yes