Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11608 )
Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider ...................................................................... Patch Set 4: (12 comments) Agreed that spreading this retry pattern is not pretty. Some thoughts: - is it an improvement when compared to the user seeing these exceptions? - perhaps it indicates that there are too many service-level methods. I'm in favor of funneling these through a common path, but I'd rather do that restructure separately. an example of this is MetadataOp which fwict is used by the hs2 entry point. Other suggestions welcome. http://gerrit.cloudera.org:8080/#/c/11608/4/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/4/fe/src/main/java/org/apache/impala/service/Frontend.java@660 PS4, Line 660: * Keeps track of retries when handling InconsistentMetadataFetchExceptions. > Maybe add more context and documentation. In which cases is this used etc. Done http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@666 PS4, Line 666: private String msg_; > final Done http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@671 PS4, Line 671: > ..of.. Done http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@680 PS4, Line 680: LOG.warn("Retried {}: {} (retry #{} of {})", msg_, exception.getMessage(), > Should any of this bubble up to the runtime profiles (if one exists) for th would make sense to handle these uniformly with queries. added a todo. that plumbling is better done separately. http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@681 PS4, Line 681: ); > , exception) to log the trace and you can probably skip exception.getMessag wasn't entirely sure what's the suggestion here, but I was aiming for consistency with the query-retry message http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py File tests/custom_cluster/test_local_catalog.py: http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@173 PS4, Line 173: def test_cache_metrics(self, unique_database): > did something change here or was it just moved to the top? Kind of confusin separated these two types of tests into two classes so this test moved up here. http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@218 PS4, Line 218: _find_inconsistent_metadata > name seems confusing. Maybe call check_metadata_fetch_retries or some such renamed to _check_metadata_retries http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@221 PS4, Line 221: , > mention the randomness part? Done http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@236 PS4, Line 236: def stress_thread(client): > Add a comment on what this does? Done http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@250 PS4, Line 250: inconsistent_seen[0] += 1 > catch and record other exceptions since this runs in a thread? Done http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@258 PS4, Line 258: 5 > I doubt if this is enough, I noticed some concurrency errors required > 60s I saw it repro at this level, but I upped in case its too close to the edge. Set it to 30, which is the level used in tests already. http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@280 PS4, Line 280: "alter table %s.%s set tblproperties ('numRows'='3')" % > Run these on 'functional' tables with a concurrent refresh? That way you ca I didn't want to modify the state (see the alter on L280). That can leak to other tests, which is a mess. -- 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: 4 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-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 17 Oct 2018 17:36:05 +0000 Gerrit-HasComments: Yes
