MPP: update the state of building a hash table when createOnce throw exceptions #4202

fzhedu · 2022-03-08T14:29:46Z

Signed-off-by: fzhedu [email protected]

What problem does this PR solve?

Issue Number: close #4195
Problem Summary:
set FinishBuild be true when createOnce throw exceptions

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Fix the issue that a query containing `JOIN` could be hung if an error was encountered

ti-chi-bot · 2022-03-08T14:29:48Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

fuzhe1989
windtalker

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

fzhedu · 2022-03-08T14:32:19Z

/run-all-tests

fuzhe1989 · 2022-03-08T14:33:31Z

dbms/src/DataStreams/CreatingSetsBlockInputStream.cpp

+    std::stringstream log_msg;
+    log_msg << std::fixed << std::setprecision(3);
+    log_msg << (subquery.set ? "Creating set. " : "")
+            << (subquery.join ? "Creating join. " : "") << (subquery.table ? "Filling temporary table. " : "") << " for task "
+            << mpp_task_id.toString();
+
+    LOG_DEBUG(log, log_msg.rdbuf());


please use fmt instead.

Agreed, and you don't have to move the log part out of the try block.

fuzhe1989 · 2022-03-08T14:33:43Z

dbms/src/DataStreams/CreatingSetsBlockInputStream.cpp

+        log_msg << " throw exception: unknown error"
+                << " In " << watch.elapsedSeconds() << " sec. ";
+        LOG_ERROR(log, log_msg.rdbuf());


ditto, use LOG_FMT_ERROR instead.

fuzhe1989 · 2022-03-08T14:37:47Z

dbms/src/DataStreams/CreatingSetsBlockInputStream.cpp

    catch (std::exception & e)
    {
        std::unique_lock<std::mutex> lock(exception_mutex);
+        log_msg << " throw exception: " << e.what() << " In " << watch.elapsedSeconds() << " sec. ";
+        LOG_ERROR(log, log_msg.rdbuf());
        exception_from_workers.push_back(std::current_exception());
+        if (subquery.join)
+            subquery.join->setFinishBuildTable(true);
+    }


guess now we don't need to catch std::exception.

why not?
we should push the exception and set finish flag here.

fuzhe1989 · 2022-03-08T14:40:02Z

dbms/src/DataStreams/CreatingSetsBlockInputStream.cpp

+                LOG_DEBUG(log, "Creat all tasks of " << mpp_task_id.toString() << " take " << watch.elapsedSeconds() << " sec with exception and rethrow the first, left " << exception_from_workers.size());
+                std::rethrow_exception(exception_from_workers.front());
+            }
+            else


when will this branch happen?

just safty check, in case some bugs

sre-bot · 2022-03-08T14:42:00Z

Coverage for changed files

Filename                                                              Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/build/tics/dbms/src/DataStreams/CreatingSetsBlockInputStream.cpp         172               172     0.00%           9                 9     0.00%         197               197     0.00%         118               118     0.00%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                     172               172     0.00%           9                 9     0.00%         197               197     0.00%         118               118     0.00%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
16777      9447             43.69%    188937  95760        49.32%

full coverage report (for internal network access only)

windtalker · 2022-03-08T23:41:09Z

dbms/src/DataStreams/CreatingSetsBlockInputStream.cpp

+    std::stringstream log_msg;
+    log_msg << std::fixed << std::setprecision(3);
+    log_msg << (subquery.set ? "Creating set. " : "")
+            << (subquery.join ? "Creating join. " : "") << (subquery.table ? "Filling temporary table. " : "") << " for task "
+            << mpp_task_id.toString();
+
+    LOG_DEBUG(log, log_msg.rdbuf());


Agreed, and you don't have to move the log part out of the try block.

windtalker · 2022-03-09T00:21:39Z

dbms/src/DataStreams/CreatingSetsBlockInputStream.cpp

+        LOG_ERROR(log, log_msg.rdbuf());
+        exception_from_workers.push_back(std::current_exception());
+        if (subquery.join)
+            subquery.join->setFinishBuildTable(true);


Why not add a flag indicating whether the build success or not in Join, so in probe stage, we don't have to do the actual probe if the build failed.

How about calling IProfilingBlockInputStream.cancel for other subqueries

How about calling IProfilingBlockInputStream.cancel for other subqueries

that will break the current cancel process. now, first tiflash report errors to tidb, then tidb sends kill command, last, the tiflash conduct the cancel command.

windtalker · 2022-03-09T00:22:18Z

Please add a test for this.

ti-chi-bot · 2022-03-14T14:53:52Z

In response to a cherrypick label: new pull request created: #4268.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2022-03-14T14:54:32Z

In response to a cherrypick label: new pull request created: #4269.

ti-chi-bot · 2022-03-14T14:55:10Z

In response to a cherrypick label: new pull request created: #4270.

…exceptions (pingcap#4202) close pingcap#4195

…exceptions (#4202) close #4195

…exceptions (pingcap#4202) close pingcap#4195

…exceptions (#4202) (#4659) close #4195

…exceptions (#4202) (#4268) close #4195

sre-bot · 2022-04-22T04:53:42Z

Coverage for changed files

Filename                                         Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/FailPoint.cpp                                 416                91    78.12%           6                 0   100.00%          56                 8    85.71%         138                58    57.97%
DataStreams/CreatingSetsBlockInputStream.cpp         186               186     0.00%           9                 9     0.00%         178               178     0.00%         118               118     0.00%
Interpreters/Join.cpp                               1276              1276     0.00%          79                79     0.00%        1410              1410     0.00%         856               856     0.00%
Interpreters/Join.h                                   16                16     0.00%          16                16     0.00%          16                16     0.00%           0                 0         -
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                               1894              1569    17.16%         110               104     5.45%        1660              1612     2.89%        1112              1032     7.19%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
16907      9490             43.87%    190400  96266        49.44%

full coverage report (for internal network access only)

…exceptions (#4202) (#4270) close #4195

…exceptions (pingcap#4202) (pingcap#4268) close pingcap#4195

…exceptions (#4202) (#4267) close #4195

…exceptions (#4202) (#4269) close #4195

…exceptions (#4202) (#4266) close #4195

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 8, 2022

fzhedu requested review from windtalker, fuzhe1989 and SeaRise March 8, 2022 14:31

fzhedu added affects-5.0 affects-5.1 affects-5.2 affects-5.3 affects-5.4 severity/critical type/bugfix This PR fixes a bug. labels Mar 8, 2022

fzhedu removed the severity/critical label Mar 8, 2022

fuzhe1989 reviewed Mar 8, 2022

View reviewed changes

windtalker reviewed Mar 9, 2022

View reviewed changes

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Mar 14, 2022

This is an automated cherry-pick of pingcap#4202

79c0df6

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Mar 14, 2022

MPP: update the state of building a hash table when createOnce throw exceptions (#4202) #4269

Merged

12 tasks

ti-chi-bot mentioned this pull request Mar 14, 2022

MPP: update the state of building a hash table when createOnce throw exceptions (#4202) #4270

Merged

12 tasks

JaySon-Huang pushed a commit to JaySon-Huang/tiflash that referenced this pull request Mar 17, 2022

MPP: update the state of building a hash table when createOnce throw …

0c758be

…exceptions (pingcap#4202) close pingcap#4195

windtalker pushed a commit that referenced this pull request Apr 1, 2022

MPP: update the state of building a hash table when createOnce throw …

f234f3b

…exceptions (#4202) close #4195

VelocityLight added cherry-pick-approved Cherry pick PR approved by release team. do-not-merge/cherry-pick-not-approved and removed cherry-pick-approved Cherry pick PR approved by release team. do-not-merge/cherry-pick-not-approved labels Apr 10, 2022

fzhedu added a commit to fzhedu/tiflash that referenced this pull request Apr 14, 2022

MPP: update the state of building a hash table when createOnce throw …

6a7bdc3

…exceptions (pingcap#4202) close pingcap#4195

fzhedu mentioned this pull request Apr 14, 2022

MPP: update the state of building a hash table when createOnce throw … #4659

Merged

12 tasks

fzhedu added a commit that referenced this pull request Apr 14, 2022

MPP: update the state of building a hash table when createOnce throw …

eb62cd2

…exceptions (#4202) (#4659) close #4195

ti-chi-bot added a commit that referenced this pull request Apr 14, 2022

MPP: update the state of building a hash table when createOnce throw …

0b823b4

…exceptions (#4202) (#4268) close #4195

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 22, 2022

ti-chi-bot added a commit that referenced this pull request Apr 25, 2022

MPP: update the state of building a hash table when createOnce throw …

ef69b08

…exceptions (#4202) (#4270) close #4195

windtalker mentioned this pull request Jun 9, 2022

Refine error handling/cancel in TiFlash MPP system #5095

Closed

ti-chi-bot mentioned this pull request Jun 16, 2022

MPP: update the state of building a hash table when createOnce throw exceptions (#4202) (#4268) #5159

Closed

12 tasks

fzhedu pushed a commit to ti-chi-bot/tiflash that referenced this pull request Jun 16, 2022

MPP: update the state of building a hash table when createOnce throw …

c1f6ab0

…exceptions (pingcap#4202) (pingcap#4268) close pingcap#4195

ti-chi-bot added a commit that referenced this pull request Jun 17, 2022

MPP: update the state of building a hash table when createOnce throw …

c09e036

…exceptions (#4202) (#4267) close #4195

ti-chi-bot added a commit that referenced this pull request Jun 17, 2022

MPP: update the state of building a hash table when createOnce throw …

8f8929d

…exceptions (#4202) (#4269) close #4195

ti-chi-bot added a commit that referenced this pull request Jun 17, 2022

MPP: update the state of building a hash table when createOnce throw …

f3cd417

…exceptions (#4202) (#4266) close #4195

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPP: update the state of building a hash table when createOnce throw exceptions #4202

MPP: update the state of building a hash table when createOnce throw exceptions #4202

fzhedu commented Mar 8, 2022 •

edited by JaySon-Huang

Loading

ti-chi-bot commented Mar 8, 2022 •

edited

Loading

fzhedu commented Mar 8, 2022

fuzhe1989 Mar 8, 2022

windtalker Mar 8, 2022

fzhedu Mar 9, 2022

fuzhe1989 Mar 8, 2022

fzhedu Mar 9, 2022

fuzhe1989 Mar 8, 2022

fzhedu Mar 9, 2022

fuzhe1989 Mar 8, 2022

fzhedu Mar 9, 2022

fzhedu Mar 9, 2022

sre-bot commented Mar 8, 2022

windtalker Mar 8, 2022

windtalker Mar 9, 2022

SeaRise Mar 9, 2022

fzhedu Mar 9, 2022

windtalker commented Mar 9, 2022

ti-chi-bot commented Mar 14, 2022

ti-chi-bot commented Mar 14, 2022

ti-chi-bot commented Mar 14, 2022

sre-bot commented Apr 22, 2022

MPP: update the state of building a hash table when createOnce throw exceptions #4202

MPP: update the state of building a hash table when createOnce throw exceptions #4202

Conversation

fzhedu commented Mar 8, 2022 • edited by JaySon-Huang Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 8, 2022 • edited Loading

fzhedu commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sre-bot commented Mar 8, 2022

Coverage for changed files

Coverage summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

windtalker commented Mar 9, 2022

ti-chi-bot commented Mar 14, 2022

ti-chi-bot commented Mar 14, 2022

ti-chi-bot commented Mar 14, 2022

sre-bot commented Apr 22, 2022

Coverage for changed files

Coverage summary

fzhedu commented Mar 8, 2022 •

edited by JaySon-Huang

Loading

ti-chi-bot commented Mar 8, 2022 •

edited

Loading