Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11608 )
Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider ...................................................................... Patch Set 1: (5 comments) Haven't looked at the tests yet but some high level comments. http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java@52 PS1, Line 52: implements MetaProvider Doesn't this only apply to CatalogdMetaProvider impl? Wondering why we need retries around something like a DirectMetaProvider. http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java@100 PS1, Line 100: exception Can we say that it exceeded num retries? http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java@118 PS1, Line 118: } Should we include the number of attempts it took for a call to be successful? http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java@126 PS1, Line 126: while (true) { : try { : return provider_.loadTableNames(dbName); : } catch (InconsistentMetadataFetchException e) { : tracker.handleRetryOrThrow(e); : } can we factor out while(true) try {} catch {retry} into a helper? I think we can use reflection to invoke a method by name. retryMethod(tracker, "loadTableNames", params...) http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/service/Frontend.java@834 PS1, Line 834: getCatalog It looks like the callers should now understand whether an operation should be retryable or not and then "get" a Catalog accordingly. This seems a bit confusing to me. Is there an easy way to fix that? What if someone adds a new operation and gets a wrong type of Catalog in the future? Should we think about that? -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Comment-Date: Tue, 09 Oct 2018 18:57:29 +0000 Gerrit-HasComments: Yes
