[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Impala Public Jenkins 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 5: Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5316/ -- 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: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Tue, 11 Feb 2020 12:39:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Impala Public Jenkins 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 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5316/ DRY_RUN=true -- 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: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Tue, 11 Feb 2020 07:45:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Impala Public Jenkins 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 5: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5312/ -- 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: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Mon, 10 Feb 2020 22:34:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Impala Public Jenkins 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 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5312/ DRY_RUN=false -- 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: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Mon, 10 Feb 2020 17:30:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Impala Public Jenkins 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 5: Code-Review+2 -- 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: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Mon, 10 Feb 2020 17:30:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Andrew Sherman 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 4: Code-Review+2 LGTM, thanks -- 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: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Mon, 10 Feb 2020 17:28:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Anurag Mantripragada 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 4: Code-Review+1 LGTM. -- 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: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Mon, 10 Feb 2020 05:20:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
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 4: (1 comment) Thank you for the review again. Fixed the code style nit. http://gerrit.cloudera.org:8080/#/c/15173/3/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/3/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@79 PS3, Line 79: + " > Line 79 and 80 should have 4 space indent. I know these can be difficult to Installed the google style, thank you. However, for some reason clang-format and with this style IntelliJ double idents when a parameter is split into multiple lines. I will keep an eye on the style around the change, also will try configure this option in IntelliJ. -- 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: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Sat, 08 Feb 2020 10:59:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Impala Public Jenkins 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 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5178/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Sat, 08 Feb 2020 10:39:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Hello Andrew Sherman, Anurag Mantripragada, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15173 to look at the new patch set (#4). Change subject: IMPALA-8852: Skip short-circuit config check for coordinator-only mode .. IMPALA-8852: Skip short-circuit config check for coordinator-only mode ImpalaD should not abort when running in coordinator-only mode and DataNode is not available on the host. This change adds a condition to skip the short- circuit socket path directory checks when ImpalaD is started with 'is_executor=false' flag. Testing: Added test to JniFrontendTest.java to verify the short-circuit directory check is skipped if ImpalaD is started in coordinator-only mode. Change-Id: I373d4037f4cee203322a398b77b75810ba708bb5 --- M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java 2 files changed, 33 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/15173/4 -- 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: newpatchset Gerrit-Change-Id: I373d4037f4cee203322a398b77b75810ba708bb5 Gerrit-Change-Number: 15173 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Anurag Mantripragada 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 3: Code-Review+1 (1 comment) LGTM. Okay for Andrew to +2 after code style nit is addressed. http://gerrit.cloudera.org:8080/#/c/15173/3/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/3/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@79 PS3, Line 79: Line 79 and 80 should have 4 space indent. I know these can be difficult to catch but the best way is to teach the IDE . You can use something like https://github.com/google/styleguide/blob/gh-pages/intellij-java-google-style.xml and modify it to comply with our guidelines. (I only had to change line wrap at 90.) If I'm unsure, I usually look at the surrounding code and follow that style. -- 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: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Sat, 08 Feb 2020 02:30:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Andrew Sherman 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 3: Code-Review+1 LGTM Giving +1 for now, I can +2/commit once Anurag is happy -- 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: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Sat, 08 Feb 2020 01:36:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Impala Public Jenkins 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 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5157/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Fri, 07 Feb 2020 18:51:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
Tamas Mate has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/15173 ) Change subject: IMPALA-8852: Skip short-circuit config check for coordinator-only mode .. IMPALA-8852: Skip short-circuit config check for coordinator-only mode ImpalaD should not abort when running in coordinator-only mode and DataNode is not available on the host. This change adds a condition to skip the short- circuit socket path directory checks when ImpalaD is started with 'is_executor=false' flag. Testing: Added test to JniFrontendTest.java to verify the short-circuit directory check is skipped if ImpalaD is started in coordinator-only mode. Change-Id: I373d4037f4cee203322a398b77b75810ba708bb5 --- M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java 2 files changed, 33 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/15173/3 -- 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: newpatchset Gerrit-Change-Id: I373d4037f4cee203322a398b77b75810ba708bb5 Gerrit-Change-Number: 15173 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate
[Impala-ASF-CR] IMPALA-8852: Skip short-circuit config check for coordinator-only mode
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
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tamas Mate
Gerrit-Comment-Date: Fri, 07 Feb 2020 18:04:19 +
Gerrit-HasComments: Yes
