Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611072024.uA7KO7xp016788@ip-10-146-233-104.ec2.internal>
Date: Mon, 7 Nov 2016 20:24:07 +0000
From: "Lars Volker (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>
Reply-To: lv@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3202=3A_refactor_scratch_file_management_into_TmpFileMgr=0A?=
In-Reply-To: <gerrit.1478015815000.I0c56c195f3f28d520034f8c384494e566635fc62@gerrit.cloudera.org>
References: <gerrit.1478015815000.I0c56c195f3f28d520034f8c384494e566635fc62@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Mon, 07 Nov 2016 20:24:12 -0000

Lars Volker has posted comments on this change.

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


Patch Set 3: Code-Review+1

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4898/3/be/src/runtime/buffered-block-mgr.h
File be/src/runtime/buffered-block-mgr.h:

Line 508:   /// is
nit: line wrapping


http://gerrit.cloudera.org:8080/#/c/4898/1/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:

PS1, Line 296: x_ = ran
> Good point, users have been confused by this before when the mixed terminol
I agree that replacing Tmp with Scratch might not be worth it. Having those two works for me.


Line 336:     } else if (status.code() == TErrorCode::SCRATCH_LIMIT_EXCEEDED) {
> I prefer it this way since it's more obvious that it's an "successful" retu
Good point.


http://gerrit.cloudera.org:8080/#/c/4898/1/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:

PS1, Line 136: unique_i
> I don't think it will ever be anything asides from the query id, but maybe 
Thanks. When I first saw query_id I assumed there might be some side effects that require query id semantics, so unique_id seems clearer to me.


-- 
To view, visit http://gerrit.cloudera.org:8080/4898
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c56c195f3f28d520034f8c384494e566635fc62
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes