impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3552: make incremental stats max serialized size configurable
Date Thu, 20 Oct 2016 22:05:58 GMT
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 <yonghyun@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-HasComments: Yes

Mime
View raw message