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 <[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
