[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode

["Tamas Mate (Code Review)" <ge...@cloudera.org>]

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 <tm...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Feb 2020 18:04:19 +0000
Gerrit-HasComments: Yes