Bharath Vissapragada 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) Like we discussed offline, I'm wondering if this can be done is any other way without a major refactoring of code. Basically to enforce that the retries are done where needed (especially for stuff that can potentially be added in the future). I can't think of any other fancy ways. Todd, do you have any thoughts? 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. etc. 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 http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@671 PS4, Line 671: ..of.. 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 these requests? 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.getMessage() 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 confusing in the gerrit view. 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 thing? http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@221 PS4, Line 221: , mention the randomness part? 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? 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? 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 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 can probably avoid these unique_db, table_name creation in find_inconsistent_...() -- 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 06:31:48 +0000 Gerrit-HasComments: Yes
