Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/15173 )
Change subject: IMPALA-8852: Skip short-circuit config check for coordinator-only mode ...................................................................... Patch Set 2: (11 comments) Thank you for the review and pointers, Anurag, Andrew. It looks better now. Skipped the whole check earlier because it seemed reasonable, noticed that keeping the earlier checks is useful, incorrect clusterwide hdfs configuration will prevent only coordinator starts. http://gerrit.cloudera.org:8080/#/c/15173/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15173/2//COMMIT_MSG@7 PS2, Line 7: Skipping > Nit: Change to Skip? Done http://gerrit.cloudera.org:8080/#/c/15173/2//COMMIT_MSG@10 PS2, Line 10: DataNode is not avaiable on the hosts. This change adds a condition to > Nit: typo: available Done http://gerrit.cloudera.org:8080/#/c/15173/2//COMMIT_MSG@13 PS2, Line 13: > Usually, our commit messages also have a section "Testing" to mention the t Thank you for the pointers, added the Testing section. http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@719 PS2, Line 719: , > Could you also mention here that this will skip the check if it is coordina Done http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@723 PS2, Line 723: if (!BackendConfig.INSTANCE.getBackendCfg().is_executor) { > Should this check move after the test for DFS_CLIENT_READ_SHORTCIRCUIT_KEY? Refactored the check to only affect the directory checks. http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@724 PS2, Line 724: LOG.info("Coordinator only instance will not read local data via " + > Nit "Coordinator-only" is clearer I think Done http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@725 PS2, Line 725: "short-circuit reads."); > We use clang-format to format Java code (as well as C++) Thanks for the pointers, installed and ran on the changed parts. http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@806 PS2, Line 806: > Nit: Extra space. Done http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java File fe/src/test/java/org/apache/impala/service/JniFrontendTest.java: http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@66 PS2, Line 66: testCheckShortCircuitReadShouldReturnErrorForInvalidConfig() > Would it be possible to consolidate this and the next into a single test? S Done http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@68 PS2, Line 68: // GIVEN > I wasn't familiar with this given-when-then style but I googled it and lear It can improve the readability of the tests. Agree, it is not in harmony with the codebase, removed them when consolidated the tests. http://gerrit.cloudera.org:8080/#/c/15173/2/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@90 PS2, Line 90: conf.setStrings("dfs.domain.socket.path", ""); > Don't we need to set dfs.client.read.shortcircuit = true here? Is the defau It was left empty to have some result from 'checkShortCircuitRead' that can be checked. In the new test I had to fill it with some value to test the new code path. -- To view, visit http://gerrit.cloudera.org:8080/15173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I373d4037f4cee203322a398b77b75810ba708bb5 Gerrit-Change-Number: 15173 Gerrit-PatchSet: 2 Gerrit-Owner: Tamas Mate <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Comment-Date: Fri, 07 Feb 2020 18:04:19 +0000 Gerrit-HasComments: Yes
