Alex Behm has posted comments on this change. Change subject: IMPALA-3552: Make incremental stats max serialized size configurable ......................................................................
Patch Set 5: (12 comments) Nice cleanup 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) Line 152: cfg.load_catalog_in_background = FLAGS_load_catalog_in_background; use the thrift setter functions 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 pattern. 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 bool here, pass the lineage_event_log_dir, and then the FE BackendConfig can implement a function computeLineage() based on that 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) 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: public static void create(TBackendConfig cfg) { Preconditions.checkNotNull(cfg); Preconditions.checkState(INSTANCE == null); INSTANCE = new TBackendConfig(cfg); } Line 43: public void setBackendConfig(TBackendConfig cfg) { indentation 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 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 -- 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 <yongh...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Huaisi Xu <h...@cloudera.com> Gerrit-Reviewer: Yonghyun Hwang Gerrit-Reviewer: Yonghyun Hwang <yongh...@cloudera.com> Gerrit-HasComments: Yes
