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, 27 Oct 2016 20:31:38 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-3552: make incremental stats max serialized size configurable
......................................................................


Patch Set 1:

(4 comments)

Haven't gone through the whole patch, but I put some design comments. Let me know what you
think.

http://gerrit.cloudera.org:8080/#/c/4867/1/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:

PS1, Line 78: catalog_class_, catalog_ctor_,
            :       load_in_background, num_metadata_loading_threads, sentry_config,
            :       FlagToTLogLevel(FLAGS_v), FlagToTLogLevel(FLAGS_non_impala_java_vlog),
            :       auth_to_local, principal
How about passing a TBackendConfig directly here. Basically make the constructor's signature
accept a TBackendConfig instead of passing so many variables.


http://gerrit.cloudera.org:8080/#/c/4867/1/common/thrift/Types.thrift
File common/thrift/Types.thrift:

Line 233: struct TBackendConfig {
This doesn't seem to be the ideal place but I don't see any good place to include this. May
be Alex/Dimitris knows.


http://gerrit.cloudera.org:8080/#/c/4867/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

PS1, Line 83: boolean loadInBackground, int numMetadataLoadingThreads,
            :       String sentryServiceConfig, int impalaLogLevel, int otherLogLevel,
            :       boolean allowAuthToLocal, String kerberosPrincipal
How about moving all these into TBackendConfig?


http://gerrit.cloudera.org:8080/#/c/4867/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

PS1, Line 120: boolean lazy, String serverName, String authorizationPolicyFile,
             :       String sentryConfigFile, String authPolicyProviderClass, int impalaLogLevel,
             :       int otherLogLevel, boolean allowAuthToLocal, String defaultKuduMasterHosts
Same, how about we make TBackendConfig more generic and refactor all the configs so far? Also
look for RuntimeEnv and TStartupOptions, that can be merged with this as well.


-- 
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: 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 <yonghyun@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message