Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9626 )
Change subject: IMPALA-6488: removes use-after-free bug in lib_cache ...................................................................... Patch Set 4: Code-Review+2 (2 comments) David, does the test look okay to you now? http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.h File be/src/runtime/lib-cache.h: http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.h@156 PS4, Line 156: loading_status This comment still seems inaccurate since this routine doesn't use loading_status to propagate the error to the caller. the status is propagated to the caller by the return value, and it's the caller's caller that ultimately sticks it in loading_status to propagate it to other threads (and ultimately might be noticed by this routine in a subsequent invokation, but then in that case, a null entry is returned). (And strangely we end up calling RemoveEntry() in both cases, but just in different places). How about we just delete this sentence since I don't think it's possible to succinctly describe the interface that this code follows (a pre-existing problem)? http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.cc File be/src/runtime/lib-cache.cc: http://gerrit.cloudera.org:8080/#/c/9626/4/be/src/runtime/lib-cache.cc@325 PS4, Line 325: // other newline glitch -- To view, visit http://gerrit.cloudera.org:8080/9626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e Gerrit-Change-Number: 9626 Gerrit-PatchSet: 4 Gerrit-Owner: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 15 Mar 2018 03:47:54 +0000 Gerrit-HasComments: Yes
