Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-3552: make incremental stats max serialized size configurable ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/4772/1//COMMIT_MSG Commit Message: PS1, Line 7: make nit: Make PS1, Line 16: impala Impala http://gerrit.cloudera.org:8080/#/c/4772/1/be/src/common/global-flags.cc File be/src/common/global-flags.cc: PS1, Line 119: inc_stat_max_size whats the unit here? May be rename it to incremental_stats_max_size_mb or incremental_stats_max_size_bytes to make it more self explanatory? http://gerrit.cloudera.org:8080/#/c/4772/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java File fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java: PS1, Line 289: MAX_INCREMENTAL_STATS_SIZE_BYTES, RuntimeEnv.INSTANCE.incStatMaxSize() Please replace with --incremental_stats_size_<unit> http://gerrit.cloudera.org:8080/#/c/4772/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: PS1, Line 169: MAX_INCREMENTAL_STATS_SIZE_BYTES - Actually implementing this way seems to be little confusing to me. Reason being, this variable is set only set in the Catalog code and remains un-initialized in the frontend code. Given we share classes between the Catalog and fe code, I think its confusing to the readers why we source it from RuntimeEnv at one place and MAX_INCREMENTAL_STATS_SIZE_BYTES in the other. - Initially when you asked my suggestion on the best way to pass this flag, I thought its only being passed to the Catalog and I suggested not to use the BackendConfig way. Now that I read the patch, I realized this is being used in the ComputeStats as well. How about falling back to the BackendConfig way to do this? Although its hacky, its easier to read and it can be set from both JniCatalog and JniFrontend (Look for --load_auth_to_local flag which does the same). - Sorry for the back and forth on this but after reading the patch, I think the hacky BackendConfig is more readable. Alex/Huaisi/Yonghyun thoughts? -- To view, visit http://gerrit.cloudera.org:8080/4772 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d189ffc1c56e7a4e27e6233e9119250a2395a19 Gerrit-PatchSet: 1 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-HasComments: Yes
