[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. IMPALA-8572: Log query events before unregister. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle. Moved the event logging related code into ClientRequestState for easier code refactoring. The conditions under which the events are logged are slightly modified by this patch. Without the patch, events are logged for unsuccessful queries if atleast a single fetch is perfomed. This patch relaxes this guarantee to log events for any query that reaches the FINISHED state (rows are available to fetch by the client) and does not wait for a fetch to be performed. This simplifies the coordinator state machine by avoiding unnecessary synchronization. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Reviewed-on: http://gerrit.cloudera.org:8080/14143 Reviewed-by: Bharath Vissapragada Tested-by: Impala Public Jenkins --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,018 insertions(+), 243 deletions(-) Approvals: Bharath Vissapragada: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 13 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 12: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4544/ : 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/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 12 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Wed, 11 Sep 2019 22:27:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 12: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4938/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 12 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Wed, 11 Sep 2019 21:52:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 12: Code-Review+2 (1 comment) Carrying +2. I've spent some time on the patch to see what could be wrong with synchronization but I didn't see anything wrong. Also, interestingly the tests started working in the last 2 runs. I'll try to get this in for now. Will revert and dig into it if we run into any issues in the builds. http://gerrit.cloudera.org:8080/#/c/14143/11/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14143/11/be/src/service/client-request-state.cc@780 PS11, Line 780: // Some metadata operations like GET_COLUMNS do not rely on WaitAsync() to launch > Do we need the wait_thread_ check though? Since after the wait thread is de Like we discussed, some codepaths like metadata op requests do not launch the async wait thread but still BlockOnWait() during fetches. In such cases the condition guards them from blocking forever. We can probably refactor the code better to make the metadata ops act like regular query paths. For now, I added a comment. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 12 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Wed, 11 Sep 2019 21:51:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Hello radford nguyen, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14143 to look at the new patch set (#12). Change subject: IMPALA-8572: Log query events before unregister. .. IMPALA-8572: Log query events before unregister. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle. Moved the event logging related code into ClientRequestState for easier code refactoring. The conditions under which the events are logged are slightly modified by this patch. Without the patch, events are logged for unsuccessful queries if atleast a single fetch is perfomed. This patch relaxes this guarantee to log events for any query that reaches the FINISHED state (rows are available to fetch by the client) and does not wait for a fetch to be performed. This simplifies the coordinator state machine by avoiding unnecessary synchronization. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,018 insertions(+), 243 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/14143/12 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 12 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 11 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Wed, 11 Sep 2019 20:09:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4935/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 11 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Wed, 11 Sep 2019 08:49:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 11: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 11 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Wed, 11 Sep 2019 05:08:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 11: (1 comment) The changes look good, will wait to hear what you find out from debugging. http://gerrit.cloudera.org:8080/#/c/14143/11/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14143/11/be/src/service/client-request-state.cc@780 PS11, Line 780: if (wait_thread_.get() == nullptr) return; Do we need the wait_thread_ check though? Since after the wait thread is destroyed is_wait_done_ will still be true. And I think we shouldn't be calling this before the wait thread is created (or if we do, we should block here). -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 11 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Wed, 11 Sep 2019 04:07:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4534/ : 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/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 11 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Wed, 11 Sep 2019 01:08:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4931/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 11 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Wed, 11 Sep 2019 00:51:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/14143 )
Change subject: IMPALA-8572: Log query events before unregister.
..
Patch Set 11:
(5 comments)
Still not clear on the test failures. Addressing the comments and kicking off
another run while I look into it.
http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@749
PS10, Line 749: if (wait_thread_.get() != nullptr) {
> Should we drop the lock while joining on the wait thread? I feel like there
Done
http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@754
PS10, Line 754: // flush log events to audit authorization errors, if any.
> We could probably also drop the lock when calling this.
Done
http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@780
PS10, Line 780: if (wait_thread_.get() == nullptr) return;
> I think we can actually just delete this line, the next line is sufficient.
Done
http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@1395
PS10, Line 1395: Status status;
> Accessing query_status without acquiring a lock might not actually be safe
Done
http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@1426
PS10, Line 1426: }
> Should be a const TExecRequest& to avoid copying.
nice find.
--
To view, visit http://gerrit.cloudera.org:8080/14143
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e
Gerrit-Change-Number: 14143
Gerrit-PatchSet: 11
Gerrit-Owner: Bharath Vissapragada
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: radford nguyen
Gerrit-Comment-Date: Wed, 11 Sep 2019 00:51:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Hello radford nguyen, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14143 to look at the new patch set (#11). Change subject: IMPALA-8572: Log query events before unregister. .. IMPALA-8572: Log query events before unregister. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle. Moved the event logging related code into ClientRequestState for easier code refactoring. The conditions under which the events are logged are slightly modified by this patch. Without the patch, events are logged for unsuccessful queries if atleast a single fetch is perfomed. This patch relaxes this guarantee to log events for any query that reaches the FINISHED state (rows are available to fetch by the client) and does not wait for a fetch to be performed. This simplifies the coordinator state machine by avoiding unnecessary synchronization. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,016 insertions(+), 243 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/14143/11 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 11 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 10: (5 comments) Left a few more comments while trying to understand the test failure. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@749 PS10, Line 749: wait_thread_->Join(); Should we drop the lock while joining on the wait thread? I feel like there's some risk of introducing a deadlock if Wait() ever acquires the lock. I think this should be safe since no other thread should be concurrently modifyin wait_thread_. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@754 PS10, Line 754: LogQueryEvents(); We could probably also drop the lock when calling this. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@780 PS10, Line 780: if (wait_thread_.get() == nullptr || is_wait_done_) return; I think we can actually just delete this line, the next line is sufficient. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@1395 PS10, Line 1395: Status status = query_status(); Accessing query_status without acquiring a lock might not actually be safe now - I think it previous used to work because this code ran *after* the query status could no longer change. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@1426 PS10, Line 1426: const TExecRequest request = exec_request(); Should be a const TExecRequest& to avoid copying. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 23:01:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 10: Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4923/ -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 21:37:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 10: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4923/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 17:26:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 10: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4920/ -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 11:21:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4519/ : 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/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 07:50:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 10: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4920/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 07:09:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Hello radford nguyen, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14143 to look at the new patch set (#10). Change subject: IMPALA-8572: Log query events before unregister. .. IMPALA-8572: Log query events before unregister. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle. Moved the event logging related code into ClientRequestState for easier code refactoring. The conditions under which the events are logged are slightly modified by this patch. Without the patch, events are logged for unsuccessful queries if atleast a single fetch is perfomed. This patch relaxes this guarantee to log events for any query that reaches the FINISHED state (rows are available to fetch by the client) and does not wait for a fetch to be performed. This simplifies the coordinator state machine by avoiding unnecessary synchronization. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,009 insertions(+), 243 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/14143/10 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: Oh, yes, that's a whole lot less scary than incorrect results :) -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 05:29:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: The issue is that the lineage test does queries like following. It is changing the snapshot state as the test runs, facepalm. 04:01:31.431 insert into table functional.alltypessmall (smallint_col, int_col) 04:01:31.431 partition (year=2009, month=04) 04:01:31.431 select smallint_col, int_col 04:01:31.431 from functional.alltypes 04:01:31.431 where year=2009 and month=05; -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 04:26:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: Yes, those mismatched results are definitely weird. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 03:34:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: The other two test failures were actually stranger - some seemingly unrelated queries returned different results. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 00:20:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: I didn't get a chance to debug it but my theory at this point is that my local data (and schema) snapshot with which I generated the test file could be stale. I'll take a look later today. Thanks for the reviews. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 00:18:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: The test failure is very strange - were you able to debg at all? -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 00:16:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 10 Sep 2019 00:15:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4901/ -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Sun, 08 Sep 2019 01:18:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4493/ : 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/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Sat, 07 Sep 2019 21:49:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/14143/8/testdata/workloads/functional-query/queries/QueryTest/lineage.test File testdata/workloads/functional-query/queries/QueryTest/lineage.test: http://gerrit.cloudera.org:8080/#/c/14143/8/testdata/workloads/functional-query/queries/QueryTest/lineage.test@268 PS8, Line 268: test_haha_e4f3f13c > Yes, this is the reason the precommit failed. Fixed it. Sorry, I meant you caught the issue correctly. I fixed it. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Sat, 07 Sep 2019 21:15:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@91 PS8, Line 91: /// Since this is a blocking operation, it is invoked from a separate thread > Mention that this log query events. Done http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@96 PS8, Line 96: /// on Wait() are signalled *before* logging the events so that they can resume their > This comment needs to be updated, since we're waiting for wait_thread_ to s Done http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@333 PS8, Line 333: > typo: signal Done http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@335 PS8, Line 335: /// TODO: remove and use ExecEnv::GetInstance() instead > We could probably use a CountingBarrier instead of this pair of variables a Personally I feel like this is more readable but let me know if you feel strongly about using a barrier, I can switch. http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.cc@811 PS8, Line 811: } > nit: the best practice is to signal the CV after dropping the log (otherwis Nice catch, done. http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/impala-server.cc@a642 PS8, Line 642: > I ended up pulling out the before and after code into before.cc and after.c Thanks. http://gerrit.cloudera.org:8080/#/c/14143/8/testdata/workloads/functional-query/queries/QueryTest/lineage.test File testdata/workloads/functional-query/queries/QueryTest/lineage.test: http://gerrit.cloudera.org:8080/#/c/14143/8/testdata/workloads/functional-query/queries/QueryTest/lineage.test@268 PS8, Line 268: > Do we know that these unique database names are stable across different sys Yes, this is the reason the precommit failed. Fixed it. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Sat, 07 Sep 2019 21:13:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 9: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4901/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Sat, 07 Sep 2019 21:13:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Hello radford nguyen, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14143 to look at the new patch set (#9). Change subject: IMPALA-8572: Log query events before unregister. .. IMPALA-8572: Log query events before unregister. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle. Moved the event logging related code into ClientRequestState for easier code refactoring. The conditions under which the events are logged are slightly modified by this patch. Without the patch, events are logged for unsuccessful queries if atleast a single fetch is perfomed. This patch relaxes this guarantee to log events for any query that reaches the FINISHED state (rows are available to fetch by the client) and does not wait for a fetch to be performed. This simplifies the coordinator state machine by avoiding unnecessary synchronization. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,003 insertions(+), 243 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/14143/9 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 9 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 8: (8 comments) Looks pretty good mainly had comments about comments and tests http://gerrit.cloudera.org:8080/#/c/14143/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14143/8//COMMIT_MSG@27 PS8, Line 27: Added some test coverage for coordinator side code paths for writing Thank you for addressing the test gap! http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@91 PS8, Line 91: void Wait(); Mention that this log query events. Also, can you mention that this is called only by wait_thread_ and that any other threads that want to wait for these conditions need to call BlockOnWait()? I think that relationship is a little unclear without reading the code. http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@96 PS8, Line 96: /// BlockOnWait() may be called after WaitAsync() has been called in order to wait This comment needs to be updated, since we're waiting for wait_thread_ to signal the condition variable. http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@333 PS8, Line 333: singal typo: signal http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@335 PS8, Line 335: ConditionVariable block_on_wait_cv_; We could probably use a CountingBarrier instead of this pair of variables and slightly reduce the amount of code. I think the current approach is perfectly fine though. http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.cc@811 PS8, Line 811: block_on_wait_cv_.NotifyAll(); nit: the best practice is to signal the CV after dropping the log (otherwise signalled threads might wake up, then immediately block on acquiring the lock). http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/impala-server.cc@a642 PS8, Line 642: I ended up pulling out the before and after code into before.cc and after.cc to review the moved code. Confirmed the changes were minimal and made sense. http://gerrit.cloudera.org:8080/#/c/14143/8/testdata/workloads/functional-query/queries/QueryTest/lineage.test File testdata/workloads/functional-query/queries/QueryTest/lineage.test: http://gerrit.cloudera.org:8080/#/c/14143/8/testdata/workloads/functional-query/queries/QueryTest/lineage.test@268 PS8, Line 268: test_haha_e4f3f13c Do we know that these unique database names are stable across different systems, etc? -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Sat, 07 Sep 2019 00:17:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 8: There was a test failure with mismatched lineage output that I'm looking into, but that does not seem to be related to the core life cycle changes. So, this is ready for review. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Fri, 06 Sep 2019 15:12:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 8: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4890/ -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Fri, 06 Sep 2019 09:37:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4484/ : 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/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Fri, 06 Sep 2019 06:05:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4890/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Fri, 06 Sep 2019 05:25:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Hello radford nguyen, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14143 to look at the new patch set (#8). Change subject: IMPALA-8572: Log query events before unregister. .. IMPALA-8572: Log query events before unregister. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle. Moved the event logging related code into ClientRequestState for easier code refactoring. The conditions under which the events are logged are slightly modified by this patch. Without the patch, events are logged for unsuccessful queries if atleast a single fetch is perfomed. This patch relaxes this guarantee to log events for any query that reaches the FINISHED state (rows are available to fetch by the client) and does not wait for a fetch to be performed. This simplifies the coordinator state machine by avoiding unnecessary synchronization. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,058 insertions(+), 239 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/14143/8 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 7: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4888/ -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Fri, 06 Sep 2019 04:05:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4481/ : 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/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Fri, 06 Sep 2019 01:08:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4888/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Fri, 06 Sep 2019 00:32:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4480/ : 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/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 6 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Fri, 06 Sep 2019 00:32:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Hello radford nguyen, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14143 to look at the new patch set (#7). Change subject: IMPALA-8572: Log query events before unregister. .. IMPALA-8572: Log query events before unregister. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle. Moved the event logging related code into ClientRequestState for easier code refactoring. The conditions under which the events are logged are slightly modified by this patch. Without the patch, events are logged for unsuccessful queries if atleast a single fetch is perfomed. This patch relaxes this guarantee to log events for any query that reaches the FINISHED state (rows are available to fetch by the client) and does not wait for a fetch to be performed. This simplifies the coordinator state machine by avoiding unnecessary synchronization. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,060 insertions(+), 239 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/14143/7 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/14143/6/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/14143/6/tests/common/impala_test_suite.py@646 PS6, Line 646: i > flake8: F821 undefined name 'i' aargh, vim troubles, extraneous character. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 6 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Fri, 06 Sep 2019 00:27:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4479/ : 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/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 5 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Thu, 05 Sep 2019 23:58:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/14143 )
Change subject: IMPALA-8572: Log query events before unregister.
..
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14143/6/tests/common/impala_test_suite.py
File tests/common/impala_test_suite.py:
http://gerrit.cloudera.org:8080/#/c/14143/6/tests/common/impala_test_suite.py@646
PS6, Line 646: i
flake8: E225 missing whitespace around operator
http://gerrit.cloudera.org:8080/#/c/14143/6/tests/common/impala_test_suite.py@646
PS6, Line 646: i
flake8: F821 undefined name 'i'
http://gerrit.cloudera.org:8080/#/c/14143/6/tests/common/impala_test_suite.py@646
PS6, Line 646: \
flake8: E211 whitespace before '('
--
To view, visit http://gerrit.cloudera.org:8080/14143
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e
Gerrit-Change-Number: 14143
Gerrit-PatchSet: 6
Gerrit-Owner: Bharath Vissapragada
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: radford nguyen
Gerrit-Comment-Date: Thu, 05 Sep 2019 23:52:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Hello radford nguyen, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14143 to look at the new patch set (#6). Change subject: IMPALA-8572: Log query events before unregister. .. IMPALA-8572: Log query events before unregister. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle. Moved the event logging related code into ClientRequestState for easier code refactoring. The conditions under which the events are logged are slightly modified by this patch. Without the patch, events are logged for unsuccessful queries if atleast a single fetch is perfomed. This patch relaxes this guarantee to log events for any query that reaches the FINISHED state (rows are available to fetch by the client) and does not wait for a fetch to be performed. This simplifies the coordinator state machine by avoiding unnecessary synchronization. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,061 insertions(+), 239 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/14143/6 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 6 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/14143/4//COMMIT_MSG Commit Message: PS4: > 1) the main reason to change the behaviour in this patch would be to simpli Done http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/impala_test_suite.py@335 PS5, Line 335: ; > flake8: E703 statement ends with a semicolon Done http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/impala_test_suite.py@646 PS5, Line 646: ) > flake8: E501 line too long (91 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/test_result_verifier.py File tests/common/test_result_verifier.py: http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/test_result_verifier.py@22 PS5, Line 22: import json > flake8: F401 'json' imported but unused Done http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/test_result_verifier.py@631 PS5, Line 631: def verify_lineage(expected, actual, lineage_skip_json_keys=DEFAULT_LINEAGE_SKIP_KEYS): > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/test_result_verifier.py@635 PS5, Line 635: p > flake8: E501 line too long (102 > 90 characters) Done -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Thu, 05 Sep 2019 23:44:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before unregister. .. Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/impala_test_suite.py@335 PS5, Line 335: ; flake8: E703 statement ends with a semicolon http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/impala_test_suite.py@646 PS5, Line 646: ) flake8: E501 line too long (91 > 90 characters) http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/test_result_verifier.py File tests/common/test_result_verifier.py: http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/test_result_verifier.py@22 PS5, Line 22: import json flake8: F401 'json' imported but unused http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/test_result_verifier.py@631 PS5, Line 631: def verify_lineage(expected, actual, lineage_skip_json_keys=DEFAULT_LINEAGE_SKIP_KEYS): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/14143/5/tests/common/test_result_verifier.py@635 PS5, Line 635: p flake8: E501 line too long (102 > 90 characters) -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 5 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Thu, 05 Sep 2019 23:39:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before unregister.
Hello radford nguyen, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14143 to look at the new patch set (#5). Change subject: IMPALA-8572: Log query events before unregister. .. IMPALA-8572: Log query events before unregister. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle. Moved the event logging related code into ClientRequestState for easier code refactoring. The conditions under which the events are logged are slightly modified by this patch. Without the patch, events are logged for unsuccessful queries if atleast a single fetch is perfomed. This patch relaxes this guarantee to log events for any query that reaches the FINISHED state (rows are available to fetch by the client) and does not wait for a fetch to be performed. This simplifies the coordinator state machine by avoid unnecessary synchronization. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,059 insertions(+), 239 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/14143/5 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 5 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen
[Impala-ASF-CR] IMPALA-8572: Log query events before Unregister() call.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before Unregister() call. .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/14143/4//COMMIT_MSG Commit Message: PS4: > 1) I've spent some time looking at it, especially reading git blame and old 1) the main reason to change the behaviour in this patch would be to simplify the code. I think it would generally be safer to log the audit events before query execution starts so that we default to the safe behaviour. 2) Yep, I see what you're saying - we use the completion of a wait thread as a signal so there's a dependency on the thread actually exiting. I think we could significantly simplify this if we change around the wait thread logic so that the wait thread runs LogQueryEvents() at the end, and use a condition variable instead of the BlockOnWait() logic. We could join on the thread in Done(). That eliminates a thread, and I think simplifying BlockOnWait() is a win too. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Thu, 05 Sep 2019 17:59:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before Unregister() call.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before Unregister() call. .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/14143/4//COMMIT_MSG Commit Message: PS4: > I had a couple of high level questions. 1) I've spent some time looking at it, especially reading git blame and older jiras, but I didn't totally understand why a fetch was necessary. I didn't see an obvious downside either. So I ended up preserving the existing functionality. 2) Isn't that a deadlock? FetchInternal() blocks on Wait(), Wait() blocks on LogQueryEvents() which blocks on first fetch. Am I missing something? -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Thu, 05 Sep 2019 00:18:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before Unregister() call.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before Unregister() call. .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/14143/4//COMMIT_MSG Commit Message: PS4: I had a couple of high level questions. * Do we know why the log-only-if-results-were-fetched behaviour exists? It seems simpler to me to log anything that gets into the FINISHED state, and I'm not seeing an obvious downside. * Could we do the logging inline in the Wait() thread? Would be nice to avoid creating another thread. http://gerrit.cloudera.org:8080/#/c/14143/4/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14143/4/be/src/service/client-request-state.cc@1412 PS4, Line 1412: statemens statements -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Thu, 29 Aug 2019 00:43:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before Unregister() call.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before Unregister() call. .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4395/ : 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/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 27 Aug 2019 19:24:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before Unregister() call.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before Unregister() call. .. Patch Set 4: I think this is in a decent shape that is ready for review. It could use some more test coverage (like in query failure cases leading to authorization errors) and some test perf improvements like making the new custom cluster test faster (or) not running it for all vectors (or) running it only in exhaustive mode etc. I'd like to get some initial feedback on the approach before I fix these items. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 27 Aug 2019 18:45:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8572: Log query events before Unregister() call.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 ) Change subject: IMPALA-8572: Log query events before Unregister() call. .. Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/14143/4/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/14143/4/tests/common/impala_test_suite.py@335 PS4, Line 335: ; flake8: E703 statement ends with a semicolon http://gerrit.cloudera.org:8080/#/c/14143/4/tests/common/impala_test_suite.py@646 PS4, Line 646: ) flake8: E501 line too long (91 > 90 characters) http://gerrit.cloudera.org:8080/#/c/14143/4/tests/common/test_result_verifier.py File tests/common/test_result_verifier.py: http://gerrit.cloudera.org:8080/#/c/14143/4/tests/common/test_result_verifier.py@22 PS4, Line 22: import json flake8: F401 'json' imported but unused http://gerrit.cloudera.org:8080/#/c/14143/4/tests/common/test_result_verifier.py@631 PS4, Line 631: def verify_lineage(expected, actual, lineage_skip_json_keys=DEFAULT_LINEAGE_SKIP_KEYS): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/14143/4/tests/common/test_result_verifier.py@635 PS4, Line 635: p flake8: E501 line too long (102 > 90 characters) -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 27 Aug 2019 18:44:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8572: Log query events before Unregister() call.
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14143 to look at the new patch set (#4). Change subject: IMPALA-8572: Log query events before Unregister() call. .. IMPALA-8572: Log query events before Unregister() call. Currently, the query events (audits and lineages) are logged as a part of query unregistration. This delays the event logging in cases where the Unregister() is delayed by client for some reason (ex: Hue does not call Unregister until the browser tab is closed) or the client goes away without calling Unregister and the query timeout kicks in. This patch moves this event logging to an earlier stage in the query lifecycle while still preserving the current audit/lineage logging guarantees. Moved the event logging related code into ClientRequestState for easier code refactoring. Added some test coverage for coordinator side code paths for writing lineages. fe specific lineage tests only verified the correctness of lineage created but did not test whether it was being flushed correctly to the disk. Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h A testdata/workloads/functional-query/queries/QueryTest/lineage.test M tests/common/impala_test_suite.py M tests/common/test_result_verifier.py M tests/custom_cluster/test_lineage.py M tests/unittests/test_file_parser.py M tests/util/test_file_parser.py 10 files changed, 6,106 insertions(+), 219 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/14143/4 -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins
