Dan Hecht has posted comments on this change. Change subject: IMPALA-5488: Fix handling of exclusive HDFS file handles ......................................................................
Patch Set 2: (7 comments) One idea that would make this more verifiably correct would be to use an distinct type for the exclusive file handle (e.g. ExclusiveHdfsFileHandle or conversely call the cached one CachedHdfsFileHandle). Then, you'd have to have a parallel interface: GetExclusiveHdfsFileHandle()/ReleaseExclusiveHdfsFileHandle() but the type checker would ensure that the proper interface is used (rather than relying on the caller to pass the correct true/false value). But if you think that's overkill, I can be convinced. http://gerrit.cloudera.org:8080/#/c/7181/2/be/src/runtime/disk-io-mgr-scan-range.cc File be/src/runtime/disk-io-mgr-scan-range.cc: PS2, Line 292: This will not use a cached handle. but it's still in the cache after this call, right? it must be given L301, so I think deleting this sentence would be clearer (and it's redundant with "new"). PS2, Line 304: stringstream ss; : ss << Substitute() might be cleaner, but okay to ignore. http://gerrit.cloudera.org:8080/#/c/7181/2/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: PS2, Line 1214: fh nit: fh == nullptr, per our style (no implicit conversion to bool). Line 1215: if (cache_hit) { maybe DCHECK(!require_new)? PS2, Line 1238: dummy maybe rename that to cache_hit and then DCHECK(!cache_hit); http://gerrit.cloudera.org:8080/#/c/7181/2/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: PS2, Line 777: ` nit: use ' rather than ` PS2, Line 786: ` nit: use ' rather than ` -- To view, visit http://gerrit.cloudera.org:8080/7181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c03696984285cc9ce463edd969c5149cd83a861 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
