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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Yonghyun Hwang <[email protected]> Gerrit-HasComments: Yes
