Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-3552: Make incremental stats max serialized size configurable ......................................................................
Patch Set 5: (15 comments) I've taken over this. Please check the next PS. http://gerrit.cloudera.org:8080/#/c/4867/5//COMMIT_MSG Commit Message: PS5, Line 21: set of : gflags, to frontend, > nit: replace with "to the" Done 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) Done Line 152: cfg.load_catalog_in_background = FLAGS_load_catalog_in_background; > use the thrift setter functions Done http://gerrit.cloudera.org:8080/#/c/4867/5/be/src/catalog/catalog.h 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. Done http://gerrit.cloudera.org:8080/#/c/4867/5/be/src/service/frontend.h File be/src/service/frontend.h: Line 176: Status GetFrontendConfigBytes(JNIEnv* jni_env, jbyteArray* cfg_bytes) const; > Document. Done 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 pat Made everything optional. Line 242: 3: required i32 other_log_lvl > non_impala_java_vlog Done Line 246: 5: required i64 inc_stats_size_limit_bytes > your gflag is a uint64, I suggest making the gflag signed as well Done 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. 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) Done 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: Done Line 43: public void setBackendConfig(TBackendConfig cfg) { > indentation Done 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 Done 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 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: 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
