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 <michael.sm...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Mar 2024 12:36:40 +0000 Gerrit-HasComments: Yes
