Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611032102.uA3L2bX3002422@ip-10-146-233-104.ec2.internal>
Date: Thu, 3 Nov 2016 21:02:37 +0000
From: "Yonghyun Hwang (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>,
        Bharath Vissapragada <bharathv@cloudera.com>,
        Huaisi Xu <hxu@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: Thu, 03 Nov 2016 21:02:42 -0000

Yonghyun Hwang has posted comments on this change.

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


Patch Set 2:

(16 comments)

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

PS2, Line 7: make
> nit: Make
Done


PS2, Line 9: fox
> typo
Done


PS2, Line 11: are experiencing
            : regressions especially when upgrading.
> Please expand a little on what the regression is.
Done


Line 19: The change also includes TBackendConfig to pass backend config
> Please add a note on revamp of existing query options to use the new TBacke
Done


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

PS2, Line 73:  cfg.load_catalog_in_background = FLAGS_load_catalog_in_background;
            :   cfg.num_metadata_loading_threads = FLAGS_num_metadata_loading_threads;
            :   cfg.sentry_config.assign(FLAGS_sentry_config);
            :   // auth_to_local rules are read if --load_auth_to_local_rules is set to true
            :   // and impala is kerberized.
            :   cfg.auth_to_local = FLAGS_load_auth_to_local_rules && !FLAGS_principal.empty();
            :   cfg.principal.assign(FLAGS_principal);
            :   cfg.impala_log_lvl = FlagToTLogLevel(FLAGS_v);
            :   cfg.other_log_lvl = FlagToTLogLevel(FLAGS_non_impala_java_vlog);
            :   cfg.inc_stat_max_size = FLAGS_inc_stat_max_size;
> Can you factor all this into a method, GetBackendConfig() or something simi
Done


http://gerrit.cloudera.org:8080/#/c/4867/2/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:

Line 119: DEFINE_uint64(inc_stat_max_size, 200 * (1LL<<20), "Maximum size (in bytes) of incremental "
> long line.
Done


PS2, Line 119: inc_stat_max_size
> Rename to incremental_stats_size_limit_bytes or something? That way its mor
Done


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

PS2, Line 104: TBackendConfig cfg;
             :   cfg.authorization_policy_file.assign(FLAGS_authorization_policy_file);
             :   cfg.server_name.assign(FLAGS_server_name);
             :   cfg.sentry_config.assign(FLAGS_sentry_config);
             :   // auth_to_local rules are read if --load_auth_to_local_rules is set to true
             :   // and impala is kerberized.
             :   cfg.auth_to_local = FLAGS_load_auth_to_local_rules && !FLAGS_principal.empty();
             :   cfg.authorization_policy_provider_class.assign(FLAGS_authorization_policy_provider_class);
             :   cfg.kudu_master_hosts.assign(FLAGS_kudu_master_hosts);
             :   cfg.impala_log_lvl = FlagToTLogLevel(FLAGS_v);
             :   cfg.other_log_lvl = FlagToTLogLevel(FLAGS_non_impala_java_vlog);
             :   cfg.inc_stat_max_size = FLAGS_inc_stat_max_size;
             : 
             :   jbyteArray cfg_bytes;
             :   JniLocalFrame jni_frame;
             :   ABORT_IF_ERROR(jni_frame.push(jni_env));
             :   ABORT_IF_ERROR(SerializeThriftMsg(jni_env, &cfg, &cfg_bytes));
> Same as catalog.cc, please factor this into a method.
Done


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

Line 233: struct TBackendConfig {
> Please document the usage of this thrift struct and how it is shared betwee
Done


PS2, Line 236: required
> Some of these are only used by Catalog and not fe (and vice versa). Shouldn
Done


Line 245:   12: required string kudu_master_hosts
> While you are at this, can you merge TStartupOptions (Frontend.thrift) with
Let's do this as a separate commit


http://gerrit.cloudera.org:8080/#/c/4867/2/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java:

PS2, Line 288: incStatMaxSize in bytes
> --incremental_stats_size_limit_bytes
Done


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

PS2, Line 1591: MAX_INCREMENTAL_STATS_SIZE_BYTES
> update
Done


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

PS2, Line 33: 
            :   // This is overriden by JniFrontend/JniCatalog classes with user set configuration.
            :   // TODO: Read this from backend instead of using static variables.
            :   private static boolean allowAuthToLocalRules_ = false;
            : 
            :   // Maximum size (in bytes) of incremental stats the catalog is
            :   // allowed to serialize per table. This limit is set as a safety
            :   // check, to prevent the JVM from hitting a maximum array limit of
            :   // 1GB (or OOM) while building the thrift objects to send to
            :   // impalads.
            :   private static long incStatMaxSize_ = 0;
> Instead of maintaining each config variable again, how about maintaining a 
This is a good idea. However, setAuthToLocal is used for an __instance__ for BackendConfig while setIncStatMaxSize is used for __static__ instance. In this specific case, we cannot benefit from setTBackendConfig.


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

Line 84:                                                   ImpalaException, TException {
> Fix indentation.
Done


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

Line 132:         cfg.authorization_policy_file, cfg.sentry_config, cfg.authorization_policy_provider_class);
> long line
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: 2
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