Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15914 )
Change subject: IMPALA-9435: Usability enhancements for data cache access trace ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/15914/4/be/src/runtime/io/data-cache-trace-test.cc File be/src/runtime/io/data-cache-trace-test.cc: http://gerrit.cloudera.org:8080/#/c/15914/4/be/src/runtime/io/data-cache-trace-test.cc@166 PS4, Line 166: DataCacheTrace::CacheHitStatistics replay_stats = replayer.GetReplayStatistics(); Might be good to expand on the comments in this file a bit, eg. note here why the replay stats are different from the original trace stats. http://gerrit.cloudera.org:8080/#/c/15914/4/be/src/runtime/io/data-cache-trace.h File be/src/runtime/io/data-cache-trace.h: http://gerrit.cloudera.org:8080/#/c/15914/4/be/src/runtime/io/data-cache-trace.h@50 PS4, Line 50: class DataCacheTrace { Its a little weird how this class is basically a wrapper around several other classes/structs/enums. Might be nice to make it a namespace, eg. impala::io::trace, then I think you would be able to forward declare this stuff and not have to include this header in other headers. Not a big deal if you don't want to make the changes. http://gerrit.cloudera.org:8080/#/c/15914/4/be/src/runtime/io/data-cache.cc File be/src/runtime/io/data-cache.cc: http://gerrit.cloudera.org:8080/#/c/15914/4/be/src/runtime/io/data-cache.cc@584 PS4, Line 584: !trace_replay_ Maybe add a LIKELY, here and similar places below? -- To view, visit http://gerrit.cloudera.org:8080/15914 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0f84204d8e5145f5fa8d4851d9c19ac317db168e Gerrit-Change-Number: 15914 Gerrit-PatchSet: 4 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Wed, 20 May 2020 17:49:45 +0000 Gerrit-HasComments: Yes
