Bharath Vissapragada 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 PS2, Line 9: fox typo PS2, Line 11: are experiencing : regressions especially when upgrading. Please expand a little on what the regression is. 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 TBackendConfig method. 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 similar? http://gerrit.cloudera.org:8080/#/c/4867/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: PS2, Line 119: inc_stat_max_size Rename to incremental_stats_size_limit_bytes or something? That way its more self explanatory to the users. Line 119: DEFINE_uint64(inc_stat_max_size, 200 * (1LL<<20), "Maximum size (in bytes) of incremental " long line. 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. 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 between fe and be and how the Java side is expected to read the configs. Also use one line spacing between members like elsewhere in this file. PS2, Line 236: required Some of these are only used by Catalog and not fe (and vice versa). Shouldn't these be optional? Line 245: 12: required string kudu_master_hosts While you are at this, can you merge TStartupOptions (Frontend.thrift) with this this and remove its references from RuntimeEnv and elsewhere ? You might have to make it optional since its only used in fe and not catalog. 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 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 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 reference to a static TBackendConfig object here? So all the clients need to do is - BackendConfig.setTBackendConfig(TBackendConfig) - BackendConfig.getTBackendConfig() This saves the step of calling set() on each config and also exposes the TBackendConfig object to any caller. 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. 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 -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Yonghyun Hwang Gerrit-Reviewer: Yonghyun Hwang <[email protected]> Gerrit-HasComments: Yes
