impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5488: Fix handling of exclusive HDFS file handles
Date Sat, 17 Jun 2017 00:57:22 GMT
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 <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message