You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "ankitsultana (via GitHub)" <gi...@apache.org> on 2023/03/15 06:39:32 UTC
[GitHub] [pinot] ankitsultana opened a new pull request, #10425: [multistage] Handle Stream Cancellations for Unstarted Streams
ankitsultana opened a new pull request, #10425:
URL: https://github.com/apache/pinot/pull/10425
We detected this issue due to a flaky test and there was a discussion on this in: #10417
Context: When there's an error, we try to send the error-block via MailboxSendOperator, and after it returns a cancellation is issued for the OpChain, which issues a cancellation for the `MailboxSendOperator` as well.
On looking into the flaky test (which can be reproduced on local by running it separately in IntelliJ), I found that the receiving mailbox is never initialized for the broker's receiver. On looking further I saw that calling `StreamObserver#onNext` doesn't guarantee that the stream has been created. The onNext operation is queued in a executor (see `DelayedClientCall`), and if there's a concurrent cancellation before the queued request is processed, the stream would never get created and the receiver will be stuck waiting to be initialized because neither onNext, onError or onCompleted would be called on its end.
In this patch I have introduced a stop-gap solution for this.
For the full fix I have created this issue: #10424. I have been punting writing the design doc for this. I'll try to finish it this week.
--
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 #10425: [multistage] Handle Stream Cancellations for Unstarted Streams
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10425:
URL: https://github.com/apache/pinot/pull/10425#issuecomment-1469477015
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10425?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10425](https://codecov.io/gh/apache/pinot/pull/10425?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4adde96) into [master](https://codecov.io/gh/apache/pinot/commit/3bad67db074aa7900ad811a7dd1fcaa5731afeb9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3bad67d) will **decrease** coverage by `56.47%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10425 +/- ##
=============================================
- Coverage 70.32% 13.86% -56.47%
+ Complexity 6105 259 -5846
=============================================
Files 2055 1999 -56
Lines 111389 108942 -2447
Branches 16939 16647 -292
=============================================
- Hits 78337 15100 -63237
- Misses 27566 92623 +65057
+ Partials 5486 1219 -4267
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.86% <0.00%> (+<0.01%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10425?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...apache/pinot/query/mailbox/GrpcSendingMailbox.java](https://codecov.io/gh/apache/pinot/pull/10425?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9HcnBjU2VuZGluZ01haWxib3guamF2YQ==) | `0.00% <0.00%> (-81.58%)` | :arrow_down: |
| [...uery/runtime/executor/OpChainSchedulerService.java](https://codecov.io/gh/apache/pinot/pull/10425?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9leGVjdXRvci9PcENoYWluU2NoZWR1bGVyU2VydmljZS5qYXZh) | `0.00% <0.00%> (-96.83%)` | :arrow_down: |
... and [1613 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10425/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
: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=The+Apache+Software+Foundation)
--
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] ankitsultana closed pull request #10425: [multistage] Handle Stream Cancellations for Unstarted Streams
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana closed pull request #10425: [multistage] Handle Stream Cancellations for Unstarted Streams
URL: https://github.com/apache/pinot/pull/10425
--
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] ankitsultana commented on a diff in pull request #10425: [multistage] Handle Stream Cancellations for Unstarted Streams
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10425:
URL: https://github.com/apache/pinot/pull/10425#discussion_r1136689520
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcSendingMailbox.java:
##########
@@ -84,14 +93,15 @@ public boolean isInitialized() {
public void cancel(Throwable t) {
if (_initialized.get() && !_statusObserver.isFinished()) {
LOGGER.warn("GrpcSendingMailbox={} cancelling stream", _mailboxId);
- try {
- _mailboxContentStreamObserver.onError(Status.fromThrowable(
- new RuntimeException("Cancelled by the sender")).asRuntimeException());
- } catch (Exception e) {
- // TODO: We don't necessarily need to log this since this is relatively quite likely to happen. Logging this
- // anyways as info for now so we can see how frequently this happens.
- LOGGER.info("Unexpected error issuing onError to MailboxContentStreamObserver: {}", e.getMessage());
- }
+ DELAYED_CANCEL_SCHEDULER.schedule(() -> {
+ try {
+ _mailboxContentStreamObserver.onError(Status.CANCELLED.asRuntimeException());
Review Comment:
Changed this to simply cancelled since when sender issues onError, a rst_stream packet is sent on the stream and no additional data is sent to the receiver. In other words, receiver only sees a cancellation on its end.
However when a server calls its outbound onError then the trailers have information about the Status which contains the exception message, so in those cases we should continue to pass the detailed Status.
--
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 pull request #10425: [multistage] Handle Stream Cancellations for Unstarted Streams
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10425:
URL: https://github.com/apache/pinot/pull/10425#issuecomment-1472299474
Thank you for the thorough investigation and follow-up discussion with #10424 @ankitsultana
--
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 #10425: [multistage] Handle Stream Cancellations for Unstarted Streams
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10425:
URL: https://github.com/apache/pinot/pull/10425#discussion_r1137817503
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -185,4 +187,13 @@ private void cancelOpChain(OpChain opChain, Throwable e) {
_scheduler.deregister(opChain);
}
}
+
+ private String getErrorMessage(TransferableBlock block) {
Review Comment:
let's put this in transferable block util. there's already something similar in DataBlockUtils as well
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/mailbox/GrpcMailboxServiceTest.java:
##########
@@ -294,6 +295,10 @@ public void testStreamCancellationBySender()
GrpcSendingMailbox grpcSendingMailbox =
(GrpcSendingMailbox) _mailboxService1.getSendingMailbox(mailboxId, deadlineMs);
+ // Do cancellations immediately for this test
+ grpcSendingMailbox = Mockito.spy(grpcSendingMailbox);
+ Mockito.doReturn(0L).when(grpcSendingMailbox).getCancellationDelayMs();
Review Comment:
is there any test we can add for this cancellation?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcSendingMailbox.java:
##########
@@ -84,14 +93,15 @@ public boolean isInitialized() {
public void cancel(Throwable t) {
if (_initialized.get() && !_statusObserver.isFinished()) {
LOGGER.warn("GrpcSendingMailbox={} cancelling stream", _mailboxId);
- try {
- _mailboxContentStreamObserver.onError(Status.fromThrowable(
- new RuntimeException("Cancelled by the sender")).asRuntimeException());
- } catch (Exception e) {
- // TODO: We don't necessarily need to log this since this is relatively quite likely to happen. Logging this
- // anyways as info for now so we can see how frequently this happens.
- LOGGER.info("Unexpected error issuing onError to MailboxContentStreamObserver: {}", e.getMessage());
- }
+ DELAYED_CANCEL_SCHEDULER.schedule(() -> {
+ try {
+ _mailboxContentStreamObserver.onError(Status.CANCELLED.asRuntimeException());
Review Comment:
shouldn't the onError call be propagate to the receiving side? and populate the errorContent block? how come the receiving end is still error out on timeout?
--
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] ankitsultana commented on pull request #10425: [multistage] Handle Stream Cancellations for Unstarted Streams
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on PR #10425:
URL: https://github.com/apache/pinot/pull/10425#issuecomment-1471374429
Talked to Rong offline. There are some reservations against delaying the cancellation and #10432 is an alternative. Both options are imperfect.
Closing this one and we can go with #10432 for now. I'll follow-up on #10424 this week itself. 🚀
--
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