impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3202: refactor scratch file management into TmpFileMgr
Date Sat, 05 Nov 2016 10:08:34 GMT
Lars Volker has posted comments on this change.

Change subject: IMPALA-3202: refactor scratch file management into TmpFileMgr

Patch Set 1:

File be/src/runtime/

PS1, Line 308: EXPECT_TRUE
ASSERT_TRUE? Otherwise we will hit a NPE in DeleteBlocks().
File be/src/runtime/

Line 246:       ->Init(state->io_mgr(), tmp_file_mgr, profile, parent, mem_limit, scratch_limit);
This line break looks odd. If it was done by clang-format I'd keep it, but otherwise maybe
break after Init( .
File be/src/runtime/

Line 282:   DCHECK_EQ(tmp_files_.size(), 0);
nit: DCHECK(tmp_files_.empty());

PS1, Line 290: by ignoring the return status of
             :     // NewFile().
we don't seem to actually ignore it, the comment looks wrong.

PS1, Line 296: spilling
"No scratch directories..."? We seem to use 'tmp', 'scratch', and 'spilling' interchangeably,
maybe reduce to one or two of them?

Line 335:       scratch_space_bytes_used_counter_->Add(num_bytes);
can we handle current_bytes_allocated_ here, too? That's also the only direct write access
I could find from File to a field of FileGroup, so maybe we can unfriend them, too.

Line 336:       return Status::OK();
nit: this could just be "return status".

Line 351:     err_status.MergeStatus(errs[i]);
nit: single line
File be/src/runtime/tmp-file-mgr.h:

PS1, Line 133:  
nit: double space

PS1, Line 136: query_id
does this actually have to be the query_id or could we rename it to something like filegroup_id?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c56c195f3f28d520034f8c384494e566635fc62
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-HasComments: Yes

View raw message