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 3: (10 comments) 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@656 PS2, Line 656: // Limit the write concurrency to avoid blocking the caller (which could be calling > I think we usually go with "start the arguments on a new line indented by f Done http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@870 PS2, Line 870: "); > We usually use all-caps for constants Done http://gerrit.cloudera.org:8080/#/c/13425/2/be/src/runtime/io/data-cache.cc@874 PS2, Line 874: case STORE: jw.String("S"); break; > see above, google style guide disagrees Done http://gerrit.cloudera.org:8080/#/c/13425/3/be/src/runtime/io/data-cache.cc File be/src/runtime/io/data-cache.cc: http://gerrit.cloudera.org:8080/#/c/13425/3/be/src/runtime/io/data-cache.cc@88 PS3, Line 88: "(Advanced) Collect a trace of all lookups in the data cache."); > nit: the indent here and other places in this change seems to be different Done http://gerrit.cloudera.org:8080/#/c/13425/3/be/src/runtime/io/data-cache.cc@98 PS3, Line 98: impala-cache-trace.txt > The data cache by default removes all files with names containing prefix CA it currently creates the trace file with open() flags to truncate the existing file if there is one, so it's essentially the same behavior. If a user wants to archive them before restarting, I think it's up to them. Is that what you were asking? http://gerrit.cloudera.org:8080/#/c/13425/3/be/src/runtime/io/data-cache.cc@116 PS3, Line 116: if (file_) Flush(); > Does it make sense to also call file_->Close() here or does the d'tor take dtor handles it http://gerrit.cloudera.org:8080/#/c/13425/3/be/src/runtime/io/data-cache.cc@148 PS3, Line 148: > nit: unnecessary blank line. Done http://gerrit.cloudera.org:8080/#/c/13425/3/be/src/runtime/io/data-cache.cc@186 PS3, Line 186: unique_ptr<FileLogger> underlying_logger_; : unique_ptr<kudu::AsyncLogger> logger_; > Would be nice to briefly document these variables. Done http://gerrit.cloudera.org:8080/#/c/13425/3/be/src/runtime/io/data-cache.cc@498 PS3, Line 498: if (FLAGS_data_cache_enable_tracing) { : tracer_.reset(new Tracer(path_ + "/" + TRACE_FILE_NAME)); : RETURN_IF_ERROR(tracer_->Start()); : } > I wonder how this may affect the available storage space allocated for the yea, I didn't want to add the complexity of accounting for the trace file, since as you said, this is meant for experimentation. http://gerrit.cloudera.org:8080/#/c/13425/3/be/src/runtime/io/data-cache.cc@887 PS3, Line 887: char buf[BUF_LEN]; > nit: same name as 'buf' declared outside of the scope. While functionally c 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: 3 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: Fri, 14 Jun 2019 00:30:26 +0000 Gerrit-HasComments: Yes
