Add ability to serialize input vector and sql if expression evaluation crashes #6402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for meta-velox canceled.
|
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D48891649 |
bikramSingh91
pushed a commit
to bikramSingh91/velox
that referenced
this pull request
Sep 7, 2023
…n crashes (facebookincubator#6402) Summary: This adds an experimental flag 'experimental_velox_save_input_on_fatal_signal' that when set to true, serializes the input vector data and all the SQL expressions in the ExprSet that is currently executing whenever a fatal signal is encountered. Enabling this flag makes the signal handler async signal unsafe, so it should only be used for debugging purposes. Differential Revision: D48891649
bikramSingh91
force-pushed
the
export-D48891649
branch
from
7e6735e
to
48015a5
Compare
|
This pull request was exported from Phabricator. Differential Revision: D48891649 |
bikramSingh91
pushed a commit
to bikramSingh91/velox
that referenced
this pull request
Sep 7, 2023
…n crashes (facebookincubator#6402) Summary: This adds an experimental flag 'experimental_velox_save_input_on_fatal_signal' that when set to true, serializes the input vector data and all the SQL expressions in the ExprSet that is currently executing whenever a fatal signal is encountered. Enabling this flag makes the signal handler async signal unsafe, so it should only be used for debugging purposes. Differential Revision: D48891649
bikramSingh91
force-pushed
the
export-D48891649
branch
from
48015a5
to
3a13db8
Compare
|
This pull request was exported from Phabricator. Differential Revision: D48891649 |
kgpai
reviewed
Sep 7, 2023
velox/expression/Expr.cpp
Outdated
| @@ -40,6 +41,20 @@ DEFINE_bool( | |||
| "Whether to overwrite queryCtx and force the " | |||
| "use of simplified expression evaluation path."); | |||
|
|
|||
| DEFINE_bool( | |||
| experimental_velox_save_input_on_fatal_signal, | |||
There was a problem hiding this comment.
Should we call it velox_experimental_... ? This seems closer to convention.
velox/expression/Expr.cpp
Outdated
| DEFINE_bool( | ||
| experimental_velox_save_input_on_fatal_signal, | ||
| false, | ||
| "This is an experimental flag that, when set to true, " |
There was a problem hiding this comment.
nit: Maybe write as follows: This is an experimental flag only to be used for debugging purposes . If set , it serializes...
velox/expression/Expr.cpp
Outdated
| "in directories specified by either " | ||
| "'velox_save_input_on_expression_any_failure_path' or " | ||
| "'velox_save_input_on_expression_system_failure_path', " | ||
| "so either of these should be specified to enable this."); |
There was a problem hiding this comment.
This final 'so either of these..' is redundant since you already mentioned in directories specified by either above.
| return; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Should we try and persist the sql expressions first , since its far less likely that they might have corruptions etc ?
There was a problem hiding this comment.
As discussed offline, I'll change it catch and proceed if either of these paths throw an exception
| }; | ||
| process::ScopedThreadDebugInfo scopedDebugInfo(debugInfo); | ||
|
|
||
| for (int32_t i = begin; i < end; ++i) { |
There was a problem hiding this comment.
nit: Only reason we need to duplicate the for loop is because of the scopedcDebugInfo - is there a way to make that a no-op and not have the loop twice ?
There was a problem hiding this comment.
Yes thats exactly the reason. I could not find any other way. I thought of moving the loop in a utility method, but its small enough that its not worth moving it to one.
bikramSingh91
pushed a commit
to bikramSingh91/velox
that referenced
this pull request
Sep 16, 2023
…n crashes (facebookincubator#6402) Summary: This adds an experimental flag 'experimental_velox_save_input_on_fatal_signal' that when set to true, serializes the input vector data and all the SQL expressions in the ExprSet that is currently executing whenever a fatal signal is encountered. Enabling this flag makes the signal handler async signal unsafe, so it should only be used for debugging purposes. Differential Revision: D48891649
bikramSingh91
force-pushed
the
export-D48891649
branch
from
3a13db8
to
b0b3d82
Compare
|
This pull request was exported from Phabricator. Differential Revision: D48891649 |
…n crashes (facebookincubator#6402) Summary: This adds an experimental flag 'experimental_velox_save_input_on_fatal_signal' that when set to true, serializes the input vector data and all the SQL expressions in the ExprSet that is currently executing whenever a fatal signal is encountered. Enabling this flag makes the signal handler async signal unsafe, so it should only be used for debugging purposes. Differential Revision: D48891649
bikramSingh91
force-pushed
the
export-D48891649
branch
from
b0b3d82
to
b31883c
Compare
|
This pull request was exported from Phabricator. Differential Revision: D48891649 |
kgpai
approved these changes
Sep 18, 2023
|
This pull request has been merged in 8e4b1cb. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
codyschierbeck
pushed a commit
to codyschierbeck/velox
that referenced
this pull request
Sep 27, 2023
…n crashes (facebookincubator#6402) Summary: Pull Request resolved: facebookincubator#6402 This adds an experimental flag 'experimental_velox_save_input_on_fatal_signal' that when set to true, serializes the input vector data and all the SQL expressions in the ExprSet that is currently executing whenever a fatal signal is encountered. Enabling this flag makes the signal handler async signal unsafe, so it should only be used for debugging purposes. Reviewed By: kgpai Differential Revision: D48891649 fbshipit-source-id: 47722d726c76a8602cf436c1840d2a0d720e2c35
codyschierbeck
pushed a commit
to codyschierbeck/velox
that referenced
this pull request
Sep 27, 2023
…n crashes (facebookincubator#6402) Summary: Pull Request resolved: facebookincubator#6402 This adds an experimental flag 'experimental_velox_save_input_on_fatal_signal' that when set to true, serializes the input vector data and all the SQL expressions in the ExprSet that is currently executing whenever a fatal signal is encountered. Enabling this flag makes the signal handler async signal unsafe, so it should only be used for debugging purposes. Reviewed By: kgpai Differential Revision: D48891649 fbshipit-source-id: 47722d726c76a8602cf436c1840d2a0d720e2c35
codyschierbeck
pushed a commit
to codyschierbeck/velox
that referenced
this pull request
Sep 27, 2023
…n crashes (facebookincubator#6402) Summary: Pull Request resolved: facebookincubator#6402 This adds an experimental flag 'experimental_velox_save_input_on_fatal_signal' that when set to true, serializes the input vector data and all the SQL expressions in the ExprSet that is currently executing whenever a fatal signal is encountered. Enabling this flag makes the signal handler async signal unsafe, so it should only be used for debugging purposes. Reviewed By: kgpai Differential Revision: D48891649 fbshipit-source-id: 47722d726c76a8602cf436c1840d2a0d720e2c35
ericyuliu
pushed a commit
to ericyuliu/velox
that referenced
this pull request
Oct 12, 2023
…n crashes (facebookincubator#6402) Summary: Pull Request resolved: facebookincubator#6402 This adds an experimental flag 'experimental_velox_save_input_on_fatal_signal' that when set to true, serializes the input vector data and all the SQL expressions in the ExprSet that is currently executing whenever a fatal signal is encountered. Enabling this flag makes the signal handler async signal unsafe, so it should only be used for debugging purposes. Reviewed By: kgpai Differential Revision: D48891649 fbshipit-source-id: 47722d726c76a8602cf436c1840d2a0d720e2c35
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This adds an experimental flag
'experimental_velox_save_input_on_fatal_signal' that when set to
true, serializes the input vector data and all the SQL expressions
in the ExprSet that is currently executing whenever a fatal signal
is encountered. Enabling this flag makes the signal handler async
signal unsafe, so it should only be used for debugging purposes.
Differential Revision: D48891649