You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "walterddr (via GitHub)" <gi...@apache.org> on 2023/05/25 00:14:33 UTC
[GitHub] [pinot] walterddr opened a new pull request, #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
walterddr opened a new pull request, #10802:
URL: https://github.com/apache/pinot/pull/10802
Details
===
- mailbox service thread that sends data from block exchange can get error block without sending it over problem, this PR fixes it
- improve exception messaging by adding more LOGGER in place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10802:
URL: https://github.com/apache/pinot/pull/10802#issuecomment-1562117454
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10802?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10802](https://app.codecov.io/gh/apache/pinot/pull/10802?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6b54458) into [master](https://app.codecov.io/gh/apache/pinot/commit/c4ee122df56d2fe229fde98cfe5e5643c5c8fc7b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c4ee122) will **decrease** coverage by `46.28%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10802 +/- ##
=============================================
- Coverage 70.38% 24.10% -46.28%
+ Complexity 6493 49 -6444
=============================================
Files 2158 2142 -16
Lines 116023 115637 -386
Branches 17563 17511 -52
=============================================
- Hits 81657 27873 -53784
- Misses 28676 84813 +56137
+ Partials 5690 2951 -2739
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.10% <0.00%> (-0.03%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10802?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...query/runtime/operator/exchange/BlockExchange.java](https://app.codecov.io/gh/apache/pinot/pull/10802?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9leGNoYW5nZS9CbG9ja0V4Y2hhbmdlLmphdmE=) | `0.00% <0.00%> (-90.70%)` | :arrow_down: |
| [...va/org/apache/pinot/query/service/QueryServer.java](https://app.codecov.io/gh/apache/pinot/pull/10802?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9RdWVyeVNlcnZlci5qYXZh) | `0.00% <0.00%> (-67.40%)` | :arrow_down: |
... and [1627 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10802/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr merged pull request #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10802:
URL: https://github.com/apache/pinot/pull/10802
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10802:
URL: https://github.com/apache/pinot/pull/10802#discussion_r1204883498
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java:
##########
@@ -137,6 +147,19 @@ protected void sendBlock(SendingMailbox sendingMailbox, TransferableBlock block)
}
}
+ private void setErrorBlock(TransferableBlock errorBlock) {
+ if (_errorBlock.compareAndSet(null, errorBlock)) {
+ try {
+ for (SendingMailbox sendingMailbox : _sendingMailboxes) {
Review Comment:
doesnt really matter b/c once error is sent. you don't need to reenqueue the opChain (it returned error block and will terminate the opChain)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10802:
URL: https://github.com/apache/pinot/pull/10802#discussion_r1204924246
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java:
##########
@@ -137,6 +147,19 @@ protected void sendBlock(SendingMailbox sendingMailbox, TransferableBlock block)
}
}
+ private void setErrorBlock(TransferableBlock errorBlock) {
+ if (_errorBlock.compareAndSet(null, errorBlock)) {
+ try {
+ for (SendingMailbox sendingMailbox : _sendingMailboxes) {
Review Comment:
when opChain received an error block (which will be the case when MailboxSendOperator is always the top of an opchain) it will terminate and will not re-enqueue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10802:
URL: https://github.com/apache/pinot/pull/10802#discussion_r1204924024
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java:
##########
@@ -100,6 +104,10 @@ public TransferableBlock send() {
try {
TransferableBlock block;
long timeoutMs = _deadlineMs - System.currentTimeMillis();
+ if (_errorBlock.get() != null) {
Review Comment:
not needed. just in case we can remove actually
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10802:
URL: https://github.com/apache/pinot/pull/10802#discussion_r1204874480
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java:
##########
@@ -137,6 +147,19 @@ protected void sendBlock(SendingMailbox sendingMailbox, TransferableBlock block)
}
}
+ private void setErrorBlock(TransferableBlock errorBlock) {
+ if (_errorBlock.compareAndSet(null, errorBlock)) {
+ try {
+ for (SendingMailbox sendingMailbox : _sendingMailboxes) {
Review Comment:
Should we call `_callback.accept()` here?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java:
##########
@@ -100,6 +104,10 @@ public TransferableBlock send() {
try {
TransferableBlock block;
long timeoutMs = _deadlineMs - System.currentTimeMillis();
+ if (_errorBlock.get() != null) {
Review Comment:
Do we need to cache the error block? Once error block is returned, there should be no more `send()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10802:
URL: https://github.com/apache/pinot/pull/10802#discussion_r1204903384
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java:
##########
@@ -100,6 +104,10 @@ public TransferableBlock send() {
try {
TransferableBlock block;
long timeoutMs = _deadlineMs - System.currentTimeMillis();
+ if (_errorBlock.get() != null) {
Review Comment:
My question is whether we need to cache it since there will be no more call to `send()` after error block is returned the first time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10802:
URL: https://github.com/apache/pinot/pull/10802#discussion_r1204904148
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java:
##########
@@ -137,6 +147,19 @@ protected void sendBlock(SendingMailbox sendingMailbox, TransferableBlock block)
}
}
+ private void setErrorBlock(TransferableBlock errorBlock) {
+ if (_errorBlock.compareAndSet(null, errorBlock)) {
+ try {
+ for (SendingMailbox sendingMailbox : _sendingMailboxes) {
Review Comment:
Who will terminate the opChain? Just want to ensure we don't leak opChain
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10802: [multistage][bugfix] fix execution exception not propagate through issue on mailbox
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10802:
URL: https://github.com/apache/pinot/pull/10802#discussion_r1204875674
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java:
##########
@@ -100,6 +104,10 @@ public TransferableBlock send() {
try {
TransferableBlock block;
long timeoutMs = _deadlineMs - System.currentTimeMillis();
+ if (_errorBlock.get() != null) {
Review Comment:
correct. that's why i did the same as ReceivingMailbox. let me know if this is not correct
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org