Add TaskListener #1404
Closed
Add TaskListener #1404
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
mbasmanova
requested review from
majetideepak,
spershin,
pedroerp and
amitkdutta
facebook-github-bot
added
the
CLA Signed
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
mbasmanova
force-pushed
the
task-listener
branch
from
2b12137
to
3a5a5a9
Compare
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
mbasmanova
added a commit
to mbasmanova/velox-1
that referenced
this pull request
Apr 12, 2022
Summary: Add a mechanism to register a listener to receive task completion events. This is useful for logging and debugging. Pull Request resolved: facebookincubator#1404 Differential Revision: D35579103 Pulled By: mbasmanova fbshipit-source-id: ef8537fe86ceca442e545c9e036e150f2883cf6d
mbasmanova
force-pushed
the
task-listener
branch
from
3a5a5a9
to
5b15b73
Compare
|
This pull request was exported from Phabricator. Differential Revision: D35579103 |
mbasmanova
added a commit
to mbasmanova/velox-1
that referenced
this pull request
Apr 12, 2022
Summary: Add a mechanism to register a listener to receive task completion events. This is useful for logging and debugging. Pull Request resolved: facebookincubator#1404 Differential Revision: D35579103 Pulled By: mbasmanova fbshipit-source-id: 66030ecf75571e731dc0cde2d8b10985b518bbc9
mbasmanova
force-pushed
the
task-listener
branch
from
5b15b73
to
608b388
Compare
|
This pull request was exported from Phabricator. Differential Revision: D35579103 |
|
|
||
| bool unregisterTaskListener(const std::shared_ptr<TaskListener>& listener) { | ||
| return listeners().withWLock([&](auto& listeners) { | ||
| for (auto it = listeners.begin(); it != listeners.end(); ++it) { |
There was a problem hiding this comment.
can we use for (const auto& entry : listeners) ?
There was a problem hiding this comment.
I realized erase needs an iterator.
I saw another erase-remove-idiom listeners.erase(std::remove(listeners.begin(), listeners.end(), listener), listeners.end()); here https://stackoverflow.com/questions/3385229/c-erase-vector-element-by-value-rather-than-by-position
There was a problem hiding this comment.
I don't find this one more readable. What do you think?
There was a problem hiding this comment.
Yeah, it is verbose. The current pattern is fine.
velox/exec/Task.cpp
Outdated
|
|
||
| void registerTaskListener(std::shared_ptr<TaskListener> listener) { | ||
| listeners().withWLock( | ||
| [&](auto& listeners) { listeners.push_back(std::move(listener)); }); |
There was a problem hiding this comment.
should we check if the listener is already added? It could happen due to an incorrect user application.
There was a problem hiding this comment.
We could have a check and not add a duplicate if already exists.
|
This pull request was exported from Phabricator. Differential Revision: D35579103 |
mbasmanova
added a commit
to mbasmanova/velox-1
that referenced
this pull request
Apr 12, 2022
Summary: Add a mechanism to register a listener to receive task completion events. This is useful for logging and debugging. Pull Request resolved: facebookincubator#1404 Differential Revision: D35579103 Pulled By: mbasmanova fbshipit-source-id: 89d86ad3cc10a92902febdfa93f824590cb146b5
mbasmanova
force-pushed
the
task-listener
branch
from
608b388
to
57d8f18
Compare
|
@majetideepak Deepak, thank you for review. Updated to address the comments. |
majetideepak
approved these changes
Apr 12, 2022
Summary: Add a mechanism to register a listener to receive task completion events. This is useful for logging and debugging. Pull Request resolved: facebookincubator#1404 Reviewed By: pedroerp Differential Revision: D35579103 Pulled By: mbasmanova fbshipit-source-id: f81d18b66660f04c5dc3ab5779ac8cb3232ef9c1
|
This pull request was exported from Phabricator. Differential Revision: D35579103 |
mbasmanova
force-pushed
the
task-listener
branch
from
57d8f18
to
b586d73
Compare
Closed
mbasmanova
added a commit
to mbasmanova/velox-1
that referenced
this pull request
Apr 14, 2022
Summary: Add a mechanism to register a listener to receive runtime statistics about expression evaluation at destruction of ExprSet. This is useful for logging and debugging. An earlier PR facebookincubator#1404 introduced a similar mechanism to receive runtime statistics about Task execution. Pull Request resolved: facebookincubator#1428 Differential Revision: D35656774 Pulled By: mbasmanova fbshipit-source-id: c26bf2a38eb8122324d04bf9aceee2f9e2720b0f
mbasmanova
added a commit
to mbasmanova/velox-1
that referenced
this pull request
Apr 14, 2022
Summary: Add a mechanism to register a listener to receive runtime statistics about expression evaluation at destruction of ExprSet. This is useful for logging and debugging. An earlier PR facebookincubator#1404 introduced a similar mechanism to receive runtime statistics about Task execution. Pull Request resolved: facebookincubator#1428 Reviewed By: pedroerp Differential Revision: D35656774 Pulled By: mbasmanova fbshipit-source-id: 87d17f50788106b090b3585d07d21ff401acc6be
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 15, 2022
Summary: Add a mechanism to register a listener to receive runtime statistics about expression evaluation at destruction of ExprSet. This is useful for logging and debugging. An earlier PR #1404 introduced a similar mechanism to receive runtime statistics about Task execution. Pull Request resolved: #1428 Reviewed By: pedroerp Differential Revision: D35656774 Pulled By: mbasmanova fbshipit-source-id: 6f18a75d622b44800ec6d8a28984d58cd592ac3f
Arty-Maly
pushed a commit
to Arty-Maly/velox
that referenced
this pull request
May 13, 2022
Summary: Add a mechanism to register a listener to receive runtime statistics about expression evaluation at destruction of ExprSet. This is useful for logging and debugging. An earlier PR facebookincubator#1404 introduced a similar mechanism to receive runtime statistics about Task execution. Pull Request resolved: facebookincubator#1428 Reviewed By: pedroerp Differential Revision: D35656774 Pulled By: mbasmanova fbshipit-source-id: 6f18a75d622b44800ec6d8a28984d58cd592ac3f
shiyu-bytedance
pushed a commit
to shiyu-bytedance/velox-1
that referenced
this pull request
Aug 18, 2022
Summary: Add a mechanism to register a listener to receive task completion events. This is useful for logging and debugging. Pull Request resolved: facebookincubator#1404 Reviewed By: pedroerp Differential Revision: D35579103 Pulled By: mbasmanova fbshipit-source-id: 0fd6f9a199ea151a482ef43d72c8c8c18dc547f8
shiyu-bytedance
pushed a commit
to shiyu-bytedance/velox-1
that referenced
this pull request
Aug 18, 2022
Summary: Add a mechanism to register a listener to receive runtime statistics about expression evaluation at destruction of ExprSet. This is useful for logging and debugging. An earlier PR facebookincubator#1404 introduced a similar mechanism to receive runtime statistics about Task execution. Pull Request resolved: facebookincubator#1428 Reviewed By: pedroerp Differential Revision: D35656774 Pulled By: mbasmanova fbshipit-source-id: 6f18a75d622b44800ec6d8a28984d58cd592ac3f
marin-ma
pushed a commit
to marin-ma/velox-oap
that referenced
this pull request
Dec 15, 2023
…le select from empty table/partition (facebookincubator#1345) What changes were proposed in this pull request? Bug fix no result returned in max/min/sum/count while select from empty table/partition the related pr of clickhouse backend: Kyligence/ClickHouse#424 the related issue: Kyligence/ClickHouse#425 fix issue facebookincubator#1404 How was this patch tested? by unit test
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.
Add a mechanism to register a listener to receive task completion events. This
is useful for logging and debugging.