MPP: update the state of building a hash table when createOnce throw exceptions by fzhedu · Pull Request #4202 · pingcap/tiflash

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

Signed-off-by: fzhedu fzhedu@gmail.com

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.

Details

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 <ti-community-prow-bot@tidb.io>

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 SeaRise, fuzhe1989 and windtalker March 8, 2022 14:31

fzhedu added affects-5.0 affects-5.1 affects-5.2 affects-5.3 affects-5.4 This bug affects the 5.4.x(LTS) versions. 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 <ti-community-prow-bot@tidb.io>

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

Conversation

fzhedu commented Mar 8, 2022 • edited by JaySon-Huang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fzhedu commented Mar 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sre-bot commented Mar 8, 2022

Coverage for changed files

Coverage summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windtalker commented Mar 9, 2022

Uh oh!

ti-chi-bot commented Mar 14, 2022

Uh oh!

ti-chi-bot commented Mar 14, 2022

Uh oh!

ti-chi-bot commented Mar 14, 2022

Uh oh!

sre-bot commented Apr 22, 2022

Coverage for changed files

Coverage summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fzhedu commented Mar 8, 2022 •

edited by JaySon-Huang

Loading

ti-chi-bot commented Mar 8, 2022 •

edited

Loading