-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trace metadata during task creation #10815
Conversation
✅ Deploy Preview for meta-velox ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
724a030
to
ed7f7cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duanmeng LGTM % minors.
97d582b
to
3437398
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add PR description? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duanmeng thanks for the update!
832f28d
to
fe70a21
Compare
c22f5be
to
c196c58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duanmeng LGTM. Thanks!
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
3239a53
to
0281dbb
Compare
velox/exec/trace/test/CMakeLists.txt
Outdated
Folly::folly | ||
gflags::gflags | ||
glog::glog | ||
fmt::fmt | ||
${FILESYSTEM}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you need to explicitly link against all of these? I don't see any explicit #includes
for any headers from these targets. The other targets will propagate their usage requirements, there is no need to add them manually.
velox/exec/trace/CMakeLists.txt
Outdated
@@ -13,7 +13,7 @@ | |||
# limitations under the License. | |||
|
|||
add_library(velox_query_trace_exec QueryMetadataWriter.cpp QueryTraceConfig.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the issue for the linking error. It's an issue with the monolithic library build and it's helper functions that now need to be used. Sorry we haven't communicated that well yet. #9948 took a while to merge so the targets here were added after CI was run but before it got merged, sorry for the inconvenience.
For targets that are not standalone executables like tests or fuzzers etc. you have to use velox_add_library
instead of add_library
and velox_link_libraries
instead of target_link_libraries
now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have to use velox_add_library instead of add_library and velox_link_libraries instead of target_link_libraries now.
@assignUser Do you mean that I don't need to link velox_query_trace_exec explicitly in velox/benchmarks
, velox/benchmarks/basic
, and velox/dwio/dwrf/test
if I use velox_add_library(velox_query_trace_exec...
and velox_link_libraries(velox_query_trace_exec...
in velox/exec/trace/CMakeList.txt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiaoxmeng please don't merge before this is fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you still have to link the target where you are using it. When VELOX_MONO_LIBRARY=OFF
everything behaves as if you were using the original functions so you still need those links. (especially for the tests and benchmark as those are unaffected by the mono build)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For targets that are not standalone executables like tests or fuzzers etc. you have to use velox_add_library instead of add_library and velox_link_libraries instead of target_link_libraries now.
@assignUser Thanks for your suggestion, no more linking errors and ci passed.
@xiaoxmeng merged this pull request in 763c19c. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: Create a directory named `$QueryTraceBaseDir/$taskId` when a task is initiated, if query tracing is enabled. This directory will store metadata related to the task, including the query plan node tree, query configurations, and connector properties. Part of facebookincubator#9668 Pull Request resolved: facebookincubator#10815 Reviewed By: Yuhta Differential Revision: D61808438 Pulled By: xiaoxmeng fbshipit-source-id: 57eff8f4b70405ba5c60fcd8315b025b22c2317b
Summary: Create a directory named `$QueryTraceBaseDir/$taskId` when a task is initiated, if query tracing is enabled. This directory will store metadata related to the task, including the query plan node tree, query configurations, and connector properties. Part of facebookincubator#9668 Pull Request resolved: facebookincubator#10815 Reviewed By: Yuhta Differential Revision: D61808438 Pulled By: xiaoxmeng fbshipit-source-id: 57eff8f4b70405ba5c60fcd8315b025b22c2317b
Summary: Velox can record the query metadata (query plan and configs) during task creation and input vectors of the traced operator, see #10774 and #10815. This PR adds a query replayer, it can be used to replay a query locally using the metadata and input vectors from the production environment. It supports showing the summary of a query at present, and more traced operators' replaying supports will be added in the future. Also, this PR adds two query configs `query_trace_max_bytes` and `query_trace_task_reg_exp` to constraint the record input data size and trace tasks respectively to ensure the stability of the cluster in the prod. Part of #9668 Pull Request resolved: #10897 Reviewed By: tanjialiang Differential Revision: D62336733 Pulled By: xiaoxmeng fbshipit-source-id: d196738dfa92c29fe5de67a944f652a328903814
Summary: Velox can record the query metadata (query plan and configs) during task creation and input vectors of the traced operator, see facebookincubator#10774 and facebookincubator#10815. This PR adds a query replayer, it can be used to replay a query locally using the metadata and input vectors from the production environment. It supports showing the summary of a query at present, and more traced operators' replaying supports will be added in the future. Also, this PR adds two query configs `query_trace_max_bytes` and `query_trace_task_reg_exp` to constraint the record input data size and trace tasks respectively to ensure the stability of the cluster in the prod. Part of facebookincubator#9668 Pull Request resolved: facebookincubator#10897 Reviewed By: tanjialiang Differential Revision: D62336733 Pulled By: xiaoxmeng fbshipit-source-id: d196738dfa92c29fe5de67a944f652a328903814
Create a directory named
$QueryTraceBaseDir/$taskId
when a task is initiated,if query tracing is enabled. This directory will store metadata related to the task,
including the query plan node tree, query configurations, and connector properties.
Part of #9668