[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. Also ensures MarkInactive is called in ClientRequestState when Wait() completes. Previously WaitInternal would only MarkInactive on success, leaving any failed requests in an active state until explicitly closed or the session ended. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: - updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query - modified test_retry_query_timeout to use exec_time_limit_s because queries closed by idle_timeout_s don't work with get_exec_summary Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Reviewed-on: http://gerrit.cloudera.org:8080/21074 Reviewed-by: Michael Smith Tested-by: Impala Public Jenkins --- M be/src/service/client-request-state.cc M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py M tests/custom_cluster/test_query_retries.py 7 files changed, 140 insertions(+), 52 deletions(-) Approvals: Michael Smith: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 18 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 17: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 17 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 03 Apr 2024 03:25:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 17: Code-Review+2 Carry +2 after light commit message editing. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 17 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 22:17:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Riza Suminto, Jason Fehr, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#17). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. Also ensures MarkInactive is called in ClientRequestState when Wait() completes. Previously WaitInternal would only MarkInactive on success, leaving any failed requests in an active state until explicitly closed or the session ended. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: - updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query - modified test_retry_query_timeout to use exec_time_limit_s because queries closed by idle_timeout_s don't work with get_exec_summary Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/client-request-state.cc M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py M tests/custom_cluster/test_query_retries.py 7 files changed, 140 insertions(+), 52 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/17 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 17 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 16: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 16 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 22:16:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 17: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10483/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 17 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 22:17:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Riza Suminto, Jason Fehr, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#16). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. Also ensures MarkInactive is called in ClientRequestState when Wait() completes. Previously WaitInternal would only MarkInactive on success, leaving any failed requests in an active state until explicitly closed or the session ended. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: - updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. - modified test_retry_query_timeout to use exec_time_limit_s because queries closed by idle_timeout_s don't work with get_exec_summary Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/client-request-state.cc M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py M tests/custom_cluster/test_query_retries.py 7 files changed, 140 insertions(+), 52 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/16 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 16 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 13: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10481/ -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 20:52:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 15: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/13/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/13/be/src/service/impala-server.cc@2868 PS13, Line 2868: preserved_status.MergeStatus(status); > I had thought this was the original behavior, but the original behavior was Done -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 15 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 19:35:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Riza Suminto, Jason Fehr, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#15). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. Also ensures MarkInactive is called in ClientRequestState when Wait() completes. Previously WaitInternal would only MarkInactive on success, leaving any failed requests in an active state until explicitly closed or the session ended. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/client-request-state.cc M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 6 files changed, 138 insertions(+), 50 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/15 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 15 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 13: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/13/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/13/be/src/service/impala-server.cc@2868 PS13, Line 2868: if (preserved_status.ok()) preserved_status = status; > nit: I just remember that we can merge 2 error status like this: I had thought this was the original behavior, but the original behavior was never to expire a failed query due to idle timeout. So merging status does seem useful. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 17:30:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 13: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/13/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/13/be/src/service/impala-server.cc@2868 PS13, Line 2868: if (preserved_status.ok()) preserved_status = status; nit: I just remember that we can merge 2 error status like this: status.MergeStatus(preserved_status); An example is here: https://github.com/apache/impala/blob/72732da/be/src/service/query-options.cc#L1265-L1266 It is neat to show that 2 things were happening simultaneously, but I'll defer to you on whether to do this or not. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 17:28:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 13: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 17:17:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 13: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10481/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 15:49:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 13: Code-Review+1 (1 comment) Will +2 if there is no additional feedback until tomorrow. http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc@2879 PS9, Line 2879: > Addressed. Thank you for clarifying. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 02 Apr 2024 04:09:22 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Riza Suminto, Jason Fehr, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#13). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. Also ensures MarkInactive is called in ClientRequestState when Wait() completes. Previously WaitInternal would only MarkInactive on success, leaving any failed requests in an active state until explicitly closed or the session ended. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/client-request-state.cc M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 6 files changed, 136 insertions(+), 50 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/13 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 12: (15 comments) http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@99 PS12, Line 99: = flake8: W504 line break after binary operator http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' http://gerrit.cloudera.org:8080/#/c/21074/12/tests/custom_cluster/test_query_expiration.py@116 PS12, Line 116: \ flake8: W605 invalid escape sequence '\d' -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 12 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 01 Apr 2024 23:54:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc@2879 PS9, Line 2879: > Might not have been clear earlier. I think I can write a test for this, and Addressed. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 12 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 01 Apr 2024 23:53:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Riza Suminto, Jason Fehr, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#12). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. Also ensures MarkInactive is called in ClientRequestState when Wait() completes. Previously WaitInternal would only MarkInactive on success, leaving any failed requests in an active state until explicitly closed or the session ended. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/client-request-state.cc M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 6 files changed, 132 insertions(+), 47 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/12 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 12 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc@2879 PS9, Line 2879: > Ack Might not have been clear earlier. I think I can write a test for this, and it's not working because of another bug that seriously limits the usability of this patch. Any query that ends in an exception and ends up in Waiting because the client hasn't fetched rows will never go inactive, and thus won't be cancelled by the idle timeout. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 01 Apr 2024 23:39:22 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has removed a vote on this change. Change subject: IMPALA-12602: Unregister queries on idle timeout .. Removed Code-Review+2 by Riza Suminto -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 11: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc@2879 PS9, Line 2879: > Hmm. So far I can't find a way to cause there to be an exception without un Ack -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 01 Apr 2024 23:19:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc@2879 PS9, Line 2879: > Hmm. So far I can't find a way to cause there to be an exception without un Ok, pretty sure there's another bug here. Whenever we RETURN_IF_ERROR after calling MarkActive on a ClientRequestState, we don't MarkInactive and hold onto a ref. So the query never goes inactive after encountering an exception. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 01 Apr 2024 23:19:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Riza Suminto, Jason Fehr, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#11). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 5 files changed, 111 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/11 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc@2879 PS9, Line 2879: > Is there any test where preserved_status.ok() == false? Maybe test query wi Hmm. So far I can't find a way to cause there to be an exception without unregistering the query. Maybe there's no point preserving these messages because they'll always be "Query ... expired due to client inactivity". http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py@144 PS9, Line 144: assert ' expired due to ' in str(e) > Yes, that's what should be happening. Only expiration due to idle timeout i Done -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 10 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 01 Apr 2024 23:04:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/9/be/src/service/impala-server.cc@2879 PS9, Line 2879: if (preserved_status.ok()) preserved_status = status; Is there any test where preserved_status.ok() == false? Maybe test query with FAIL debug action somewhere? -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 29 Mar 2024 18:51:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py@144 PS9, Line 144: assert ' expired due to ' in str(e) > It is a bit confusing to me that in L100, time_limit_expire_handle assert h Yes, that's what should be happening. Only expiration due to idle timeout is saved for the session; everything else still returns "Invalid or unknown query handle". -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 29 Mar 2024 18:00:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py@144 PS9, Line 144: assert ' expired due to ' in str(e) > I can add a regex to be more specific. It is a bit confusing to me that in L100, time_limit_expire_handle assert hitting "expired due to execution time limit", but upon close, it errors with "Invalid or unknown query handle" in L142. I'm guessing the other 3 handles are all hitting "expired due to inactivity" upon close then? -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 29 Mar 2024 17:37:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py@144 PS9, Line 144: assert ' expired due to ' in str(e) > Is it possible to distinguish which handle "expired due to inactivity" and I can add a regex to be more specific. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 29 Mar 2024 05:55:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/9/tests/custom_cluster/test_query_expiration.py@144 PS9, Line 144: assert ' expired due to ' in str(e) Is it possible to distinguish which handle "expired due to inactivity" and which handle "expired due to execution time limit" here? -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 29 Mar 2024 01:06:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 9: Code-Review+1 Looks good! -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Sat, 23 Mar 2024 18:36:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 9: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 22 Mar 2024 08:18:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Riza Suminto, Jason Fehr, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#9). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 5 files changed, 110 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/9 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG@13 PS3, Line 13: Adds a new structure - idle_query_statuses_ - to retain Status messages : for queries closed this way so that we can continue to return a clear : error message if the client returns and requests query status or : attempts to fetch results. This structure must be global because HS2 : server can only identify a session ID from a query handle, and the quer > Some brain dump about this topic, I don't mean to change anything in this r That could also be a reasonable way to do this. It'd be a more involved change to GetActiveQueryHandle. http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.h@181 PS7, Line 181: /// This class is partially thread-safe. To ensure freedom from deadlock, if multiple > Can you add idle_query_statuses_lock_ to this list? Done http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.cc@1439 PS7, Line 1439: ImpaladMetri > nit: can be static inline? Done http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.cc@2863 PS7, Line 2863: > nit: mark const We basically never do that. Status is a essentially a scalar type. But I suppose it doesn't hurt. http://gerrit.cloudera.org:8080/#/c/21074/7/docs/topics/impala_timeouts.xml File docs/topics/impala_timeouts.xml: http://gerrit.cloudera.org:8080/#/c/21074/7/docs/topics/impala_timeouts.xml@130 PS7, Line 130: exception details. > Please add an explanation that the status will be INACTIVE_QUERY_EXPIRED if Done -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 22 Mar 2024 04:47:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 7: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG@13 PS3, Line 13: Adds a new structure - idle_query_statuses_ - to retain Status messages : for queries closed this way so that we can continue to return a clear : error message if the client returns and requests query status or : attempts to fetch results. This structure must be global because HS2 : server can only identify a session ID from a query handle, and the quer > Done Some brain dump about this topic, I don't mean to change anything in this review: While Impala can't look up the session based on the query id in HS2 at the moment, I am not convinced that it has to be this way. The session and query id are completely independent 16 byte uuids at the moment (with 4 bytes zeroed in case of the the query id to leave space for the fragment instance id). At the same time the secret (also 16 byte uuid) is the same in the query and the session, so actually a secret->session map could allow retrieving the session without looking up the query state first. Another way would to use less bytes for the ids so that both the session + query + fragment instance id would fit to 16 bytes. http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.h@181 PS7, Line 181: /// 2. SessionState::lock Can you add idle_query_statuses_lock_ to this list? -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Thu, 21 Mar 2024 11:32:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.cc@1439 PS7, Line 1439: static int32_t nit: can be static inline? -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 20 Mar 2024 22:20:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/7/be/src/service/impala-server.cc@2863 PS7, Line 2863: status nit: mark const http://gerrit.cloudera.org:8080/#/c/21074/7/docs/topics/impala_timeouts.xml File docs/topics/impala_timeouts.xml: http://gerrit.cloudera.org:8080/#/c/21074/7/docs/topics/impala_timeouts.xml@130 PS7, Line 130: exception details. Please add an explanation that the status will be INACTIVE_QUERY_EXPIRED if the query was in a good state at expiration or will be the error status if the query was not in a good state at expiration. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jason Fehr Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Wed, 20 Mar 2024 21:54:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/21074/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/5/be/src/service/impala-server.cc@2880 PS5, Line 2880: expiration_event->query_id, move(preserved_status)); > Your right, it could have been after releasing the session lock. I'll move Done http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py@140 PS3, Line 140: ed queries always return their exception, s > Yes, I'll update the asserts to test specific queries. Done. Turns out I was mistaken about short_timeout_expire_handle, it was cancelled due to QUERY_TIMEOUT_S so it always return the exception. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Thu, 14 Mar 2024 18:44:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#7). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 5 files changed, 103 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/7 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/21074/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/5/be/src/service/impala-server.cc@2880 PS5, Line 2880: > Is it possible for the session to be closed at this point? Wouldn't this me Your right, it could have been after releasing the session lock. I'll move this to happen within the session lock above. Locking order could be a little tricky, I'll have to look into that. http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py@140 PS3, Line 140: 'Invalid or unknown query handle' in str(e) > Could this be made stricter by checking whether the handle is in [short_tim Yes, I'll update the asserts to test specific queries. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Thu, 14 Mar 2024 16:46:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/21074/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/5/be/src/service/impala-server.cc@2880 PS5, Line 2880: Is it possible for the session to be closed at this point? Wouldn't this mean that we'll add the query to idle_query_statuses_ but never remove it, as we couldn't erase it during the closing of the session? http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py@140 PS3, Line 140: 'Invalid or unknown query handle' in str(e) > __expect_expired fetches from short_timeout_expire_handle and time_limit_ex Could this be made stricter by checking whether the handle is in [short_timeout_expire_handle, time_limit_expire_handle] and expect an error message based on that? -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Wed, 13 Mar 2024 18:59:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. (
http://gerrit.cloudera.org:8080/21074 )
Change subject: IMPALA-12602: Unregister queries on idle timeout
..
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/21074/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21074/1//COMMIT_MSG@10
PS1, Line 10: as you cannot fetch results
: from a cancelled query.
> I want to look into getLog more. It may continue to return the error, and p
Done
http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG@13
PS3, Line 13: Adds a new structure - idle_query_statuses_ - to retain Status
messages
: for queries closed this way so that we can continue to return a
clear
: error message if the client returns and requests query status or
: attempts to fetch results. This structure must be global because
HS2
: server can only identify a session ID from a query handle, and
the quer
> I first tried storing them per-session, but when we want to lookup the erro
Done
http://gerrit.cloudera.org:8080/#/c/21074/3/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:
http://gerrit.cloudera.org:8080/#/c/21074/3/be/src/service/impala-beeswax-server.cc@299
PS3, Line 299: if (!status.ok() && status.code() !=
TErrorCode::INVALID_QUERY_HANDLE) {
> nit: add braces
Done
http://gerrit.cloudera.org:8080/#/c/21074/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/21074/3/be/src/service/impala-server.cc@1759
PS3, Line 1759: err;
> This looks strange to me - I would prefer to get 'it' in a line before the
Done
http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py
File tests/custom_cluster/test_query_expiration.py:
http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py@140
PS3, Line 140: 'Invalid or unknown query handle' in str(e)
> Can the old error message still occur?
__expect_expired fetches from short_timeout_expire_handle and
time_limit_expire_handle, which unregisters the queries. Only queries
unregistered due to idle query timeout save the error message for later use.
That does make me wonder if we could identify a fetch or close, and after we
returned the error message in that case erase it from storage.
--
To view, visit http://gerrit.cloudera.org:8080/21074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0
Gerrit-Change-Number: 21074
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Smith
Gerrit-Comment-Date: Thu, 07 Mar 2024 18:37:46 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#5). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. This structure must be global because HS2 server can only identify a session ID from a query handle, and the query handle no longer exists. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. The beeswax get_log RPC will not return the preserved error message or any warnings for these queries. It's also possible the summary and profile are rotated out of query log as the query is no longer inflight. This is an acceptable outcome as a client will likely not look for a log/summary/profile after it times out. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 5 files changed, 104 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/5 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/21074/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21074/1//COMMIT_MSG@10 PS1, Line 10: as you cannot fetch results : from a cancelled query. > One side affect is that we also cannot get the errors/warnings for the quer I want to look into getLog more. It may continue to return the error, and profile was still accessible from impala-shell. I'll try to clarify what the behavior will be after this change. http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG@13 PS3, Line 13: Adds a new structure - idle_query_statuses_ - to retain Status messages : for queries closed this way so that we can continue to return a clear : error message if the client returns and requests query status or : attempts to fetch results. SessionState tracks queries added to : idle_query_statuses_ so they can be cleared when the session is closed. > It is not clear to me why do we need two states - would it work if idle_que I first tried storing them per-session, but when we want to lookup the error message because we could not find a handle, we don't have access to the session ID (specifically in HS2). We normally get the session ID from the query handle, and the query handle is gone by then. I'll update the commit message to explain that. -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Thu, 07 Mar 2024 17:56:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/21074/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21074/1//COMMIT_MSG@10 PS1, Line 10: as you cannot fetch results : from a cancelled query. One side affect is that we also cannot get the errors/warnings for the query with getLog. It is also possible that the summary and profile are rotated out of query log and are no longer accessible. While I agree that it is a good idea to unregister idle queries (as the client will probably never look for their log/summary/profile), these changes could be mentioned. Note that I had the same dilemma about closing queries after completion in impyla: https://github.com/cloudera/impyla/commit/b941bfcb442cf9294d5db1a7ba971ee0202b2ce0 http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21074/3//COMMIT_MSG@13 PS3, Line 13: Adds a new structure - idle_query_statuses_ - to retain Status messages : for queries closed this way so that we can continue to return a clear : error message if the client returns and requests query status or : attempts to fetch results. SessionState tracks queries added to : idle_query_statuses_ so they can be cleared when the session is closed. It is not clear to me why do we need two states - would it work if idle_query_statuses_ was stored per-session? Is the global map used in functions where the session is not available? http://gerrit.cloudera.org:8080/#/c/21074/3/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/3/be/src/service/impala-beeswax-server.cc@299 PS3, Line 299: if (!status.ok() && status.code() != TErrorCode::INVALID_QUERY_HANDLE) nit: add braces http://gerrit.cloudera.org:8080/#/c/21074/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/3/be/src/service/impala-server.cc@1759 PS3, Line 1759: auto it = idle_query_statuses_.find(query_id); This looks strange to me - I would prefer to get 'it' in a line before the if. http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/3/tests/custom_cluster/test_query_expiration.py@140 PS3, Line 140: 'Invalid or unknown query handle' in str(e) Can the old error message still occur? -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Thu, 07 Mar 2024 12:36:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#3). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 5 files changed, 102 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/3 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21074 to look at the new patch set (#2). Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure - idle_query_statuses_ - to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. SessionState tracks queries added to idle_query_statuses_ so they can be cleared when the session is closed. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 5 files changed, 102 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/2 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/21074/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/1/be/src/service/impala-server.cc@2998 PS1, Line 2998: // Save status so we can report it for unregistered queries. We could choose to do this for all queries cancelled in ExpireQueries. I think it makes sense, but that change has more surface error it might affect. Theoretically any query cancelled via ExpireQuery can no longer have results fetched, and the only interaction a client can have is to view the query's EXCEPTION status. That would more clearly delineate that anything in the "waiting to be closed" section of inflight queries is because a client is still fetching completed results (shouldn't see any queries in the EXCEPTION state in that section). -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Comment-Date: Tue, 27 Feb 2024 00:43:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Michael Smith has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21074 Change subject: IMPALA-12602: Unregister queries on idle timeout .. IMPALA-12602: Unregister queries on idle timeout Queries cancelled due to idle_query_timeout/QUERY_TIMEOUT_S are now also Unregistered to free any remaining memory, as you cannot fetch results from a cancelled query. Adds a new structure to retain Status messages for queries closed this way so that we can continue to return a clear error message if the client returns and requests query status or attempts to fetch results. Testing: updates test_query_expiration to verify number of waiting queries is only non-zero for queries cancelled by EXEC_TIME_LIMIT_S and not yet closed as an idle query. Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 --- M be/src/service/impala-beeswax-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M docs/topics/impala_timeouts.xml M tests/custom_cluster/test_query_expiration.py 5 files changed, 102 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/21074/1 -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Smith
[Impala-ASF-CR] IMPALA-12602: Unregister queries on idle timeout
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21074 ) Change subject: IMPALA-12602: Unregister queries on idle timeout .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/21074/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21074/1/be/src/service/impala-server.cc@2934 PS1, Line 2934: // Query was expired already from a previous expiration event. Keep idle timeouts line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/21074/1/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/21074/1/tests/custom_cluster/test_query_expiration.py@32 PS1, Line 32: flake8: E251 unexpected spaces around keyword / parameter equals http://gerrit.cloudera.org:8080/#/c/21074/1/tests/custom_cluster/test_query_expiration.py@32 PS1, Line 32: flake8: E251 unexpected spaces around keyword / parameter equals -- To view, visit http://gerrit.cloudera.org:8080/21074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacfc285ed3587892c7ec6f7df3b5f71c9e41baf0 Gerrit-Change-Number: 21074 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 27 Feb 2024 00:41:00 + Gerrit-HasComments: Yes
