impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3552: Make incremental stats max serialized size configurable
Date Tue, 08 Nov 2016 00:07:52 GMT
Bharath Vissapragada has posted comments on this change.

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

Patch Set 5:


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

PS5, Line 21:  set of
            : gflags, to frontend, 
> nit: replace with "to the"
File be/src/catalog/

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

Line 152:   cfg.load_catalog_in_background = FLAGS_load_catalog_in_background;
> use the thrift setter functions
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.
File be/src/service/frontend.h:

Line 176:   Status GetFrontendConfigBytes(JNIEnv* jni_env, jbyteArray* cfg_bytes) const;
> Document.
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

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

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.
File fe/src/main/java/org/apache/impala/catalog/

Line 1601:         (statsSizeEstimate < BackendConfig.INSTANCE.getIncStatMaxSize());
> getIncStatsMaxSize() (Stats vs. Stat)
File fe/src/main/java/org/apache/impala/common/

Line 30: public class RuntimeEnv {
> much better!
File fe/src/main/java/org/apache/impala/service/

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

Line 43:   public void setBackendConfig(TBackendConfig cfg) {
> indentation
File fe/src/main/java/org/apache/impala/service/

Line 83:   public JniCatalog(byte[] thriftBEConfig) throws InternalException,
> thriftBackendConfig or thriftBeConfig
File fe/src/main/java/org/apache/impala/service/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I33684725a61eabc67237503e61178305d37d3cb5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Yonghyun Hwang <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Huaisi Xu <>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: Yonghyun Hwang <>
Gerrit-HasComments: Yes

View raw message