Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611080007.uA807qKE027843@ip-10-146-233-104.ec2.internal>
Date: Tue, 8 Nov 2016 00:07:52 +0000
From: "Bharath Vissapragada (Code Review)" <gerrit@cloudera.org>
To: Yonghyun Hwang <yonghyun@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>,
        Alex Behm <alex.behm@cloudera.com>, Huaisi Xu <hxu@cloudera.com>
Reply-To: bharathv@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3552=3A_Make_incremental_stats_max_serialized_size_configurable=0A?=
In-Reply-To: <gerrit.1477592576000.I33684725a61eabc67237503e61178305d37d3cb5@gerrit.cloudera.org>
References: <gerrit.1477592576000.I33684725a61eabc67237503e61178305d37d3cb5@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: Tue, 08 Nov 2016 00:08:18 -0000

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-3552: Make incremental stats max serialized size configurable
......................................................................


Patch Set 5:

(15 comments)

I've taken over this. Please check the next PS.

http://gerrit.cloudera.org:8080/#/c/4867/5//COMMIT_MSG
Commit Message:

PS5, Line 21:  set of
            : gflags, to frontend, 
> nit: replace with "to the"
Done


http://gerrit.cloudera.org:8080/#/c/4867/5/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:

Line 150:     const {
> move into line above (90 chars per line max)
Done


Line 152:   cfg.load_catalog_in_background = FLAGS_load_catalog_in_background;
> use the thrift setter functions
Done


http://gerrit.cloudera.org:8080/#/c/4867/5/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:

Line 104:   Status GetCatalogConfigBytes(JNIEnv* jni_env, jbyteArray* cfg_bytes) const;
> Document a line or two on what this does.
Done


http://gerrit.cloudera.org:8080/#/c/4867/5/be/src/service/frontend.h
File be/src/service/frontend.h:

Line 176:   Status GetFrontendConfigBytes(JNIEnv* jni_env, jbyteArray* cfg_bytes) const;
> Document.
Done


http://gerrit.cloudera.org:8080/#/c/4867/5/common/thrift/Types.thrift
File common/thrift/Types.thrift:

Line 236:   1: required string sentry_config
> Why are some of these required and some optional? I couldn't make out a pat
Made everything optional.


Line 242:   3: required i32 other_log_lvl
> non_impala_java_vlog
Done


Line 246:   5: required i64 inc_stats_size_limit_bytes
> your gflag is a uint64, I suggest making the gflag signed as well
Done


Line 249:   6: required bool compute_lineage
> if possible, I'd prefer to pass the gflags directly, i.e., instead of a boo
Isn't it better if we let the backend drive the decision here and just pass the flags the fe? I'm fine either way but if you feel strongly about this, I'll make the change. Please let me know.


http://gerrit.cloudera.org:8080/#/c/4867/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

Line 1601:         (statsSizeEstimate < BackendConfig.INSTANCE.getIncStatMaxSize());
> getIncStatsMaxSize() (Stats vs. Stat)
Done


http://gerrit.cloudera.org:8080/#/c/4867/5/fe/src/main/java/org/apache/impala/common/RuntimeEnv.java
File fe/src/main/java/org/apache/impala/common/RuntimeEnv.java:

Line 30: public class RuntimeEnv {
> much better!
:)


http://gerrit.cloudera.org:8080/#/c/4867/5/fe/src/main/java/org/apache/impala/service/BackendConfig.java
File fe/src/main/java/org/apache/impala/service/BackendConfig.java:

Line 41:   }
> How about adding initializing the singleton this way:
Done


Line 43:   public void setBackendConfig(TBackendConfig cfg) {
> indentation
Done


http://gerrit.cloudera.org:8080/#/c/4867/5/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

Line 83:   public JniCatalog(byte[] thriftBEConfig) throws InternalException,
> thriftBackendConfig or thriftBeConfig
Done


http://gerrit.cloudera.org:8080/#/c/4867/5/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

Line 120:   public JniFrontend(byte[] thriftBEConfig) throws InternalException,
> remove InternalException because that's already covered by ImpalaException
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33684725a61eabc67237503e61178305d37d3cb5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yonghyun Hwang <yonghyun@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: Yonghyun Hwang <yonghyun@cloudera.com>
Gerrit-HasComments: Yes