[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
[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.
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: 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.
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.
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: (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
