Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12049 )
Change subject: IMPALA-4555: Make QueryState's status reporting more robust ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/12049/5/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/12049/5/be/src/runtime/fragment-instance-state.cc@259 PS5, Line 259: // Since execution was already finished, the conten > Can you please a comment on why we don't advance the seq number here to hel Done http://gerrit.cloudera.org:8080/#/c/12049/5/be/src/runtime/fragment-instance-state.cc@300 PS5, Line 300: ) != > typo ? Done http://gerrit.cloudera.org:8080/#/c/12049/5/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/12049/5/be/src/runtime/query-state.h@421 PS5, Line 421: /// Returns the amount of time in ms to wait before sending the next status report, > in milliseconds Done http://gerrit.cloudera.org:8080/#/c/12049/5/be/src/runtime/query-state.h@422 PS5, Line 422: terval with > This isn't correct, right ? Done http://gerrit.cloudera.org:8080/#/c/12049/5/be/src/runtime/query-state.h@424 PS5, Line 424: int64_t GetReportWaitTimeMs() > int64_t GetReportWaitTimeMs() const; Done http://gerrit.cloudera.org:8080/#/c/12049/5/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/12049/5/be/src/runtime/query-state.cc@415 PS5, Line 415: > A minor quirk I just noticed. If the user sets FLAGS_status_report_interval Done http://gerrit.cloudera.org:8080/#/c/12049/5/tests/custom_cluster/test_rpc_timeout.py File tests/custom_cluster/test_rpc_timeout.py: http://gerrit.cloudera.org:8080/#/c/12049/5/tests/custom_cluster/test_rpc_timeout.py@158 PS5, Line 158: # Since the sleep time (1000ms) is much longer than the rpc timeout (100ms), all : # reports will appear to fail. The query is designed to result in many intermediate : # status reports but fewer than the max allowed failures, so the query should succeed. : query_options = {'debug_action': 'REPORT_EXEC_STATUS_DELAY:SLEEP@1000'} > This is a good test. Can you please add a test which exercises the stateful Sure, happy to write such a test if you think there's additional value in it, though I'll point out that this test does exercise the stateful reporting part via the insert errors checked for below. Do you know if we have such a udf already available or would I need to write one? If it generates errors probabilistically, how do we check that the errors that are reported back were in fact the errors that were produced during the run? -- To view, visit http://gerrit.cloudera.org:8080/12049 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6007013fc2c9e8eeba11b752ee58fb3038da971 Gerrit-Change-Number: 12049 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Wed, 06 Feb 2019 20:40:56 +0000 Gerrit-HasComments: Yes
