You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/12/12 10:10:43 UTC
[GitHub] [pinot] 61yao opened a new pull request, #9969: [multistage] [bugfix] Throw error when GrpcMailbox receive buffer is full
61yao opened a new pull request, #9969:
URL: https://github.com/apache/pinot/pull/9969
We silently drop data block when GrpcMailbox receiving buffer is full today. This will return incorrect result silently.
This PR serves as a bandaid to throw error instead of returning incorrect result.
Unfortunately, it is very hard to test this until underlying mailbox error handling and async issue are fixed.
--
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] 61yao commented on a diff in pull request #9969: [multistage] [bugfix] Throw error when GrpcMailbox receiving buffer is full
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9969:
URL: https://github.com/apache/pinot/pull/9969#discussion_r1047651175
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentStreamObserver.java:
##########
@@ -93,7 +105,19 @@ public void onNext(Mailbox.MailboxContent mailboxContent) {
if (!mailboxContent.getMetadataMap().containsKey(ChannelUtils.MAILBOX_METADATA_BEGIN_OF_STREAM_KEY)) {
// when the receiving end receives a message put it in the mailbox queue.
- _receivingBuffer.offer(mailboxContent);
+ // TODO: pass a timeout to _receivingBuffer.
+ if (!_receivingBuffer.offer(mailboxContent)) {
Review Comment:
yes. Eventually, we should utilize GRPC streaming buffer for this. We need to put more thoughts into that. As said in the PR description , this PR doesn't intent to fix the issue. It serves as a quick bandaid to throw error instead of silently returning incorrect result.
--
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 #9969: [multistage] [bugfix] Throw error when GrpcMailbox receiving buffer is full
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9969:
URL: https://github.com/apache/pinot/pull/9969#discussion_r1046575539
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentStreamObserver.java:
##########
@@ -116,11 +140,9 @@ public void onNext(Mailbox.MailboxContent mailboxContent) {
@Override
public void onError(Throwable e) {
try {
- _receivingBuffer.offer(Mailbox.MailboxContent.newBuilder()
- .setPayload(ByteString.copyFrom(
- TransferableBlockUtils.getErrorTransferableBlock(new RuntimeException(e)).getDataBlock().toBytes()))
- .putMetadata(ChannelUtils.MAILBOX_METADATA_END_OF_STREAM_KEY, "true").build());
+ _errorContent = createErrorContent(e);
Review Comment:
this has to be atomic --> both onNext and onError can modify _errorContent
--
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] 61yao commented on a diff in pull request #9969: [multistage] [bugfix] Throw error when GrpcMailbox receiving buffer is full
Posted by GitBox <gi...@apache.org>.
61yao commented on code in PR #9969:
URL: https://github.com/apache/pinot/pull/9969#discussion_r1047663720
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentStreamObserver.java:
##########
@@ -116,11 +140,9 @@ public void onNext(Mailbox.MailboxContent mailboxContent) {
@Override
public void onError(Throwable e) {
try {
- _receivingBuffer.offer(Mailbox.MailboxContent.newBuilder()
- .setPayload(ByteString.copyFrom(
- TransferableBlockUtils.getErrorTransferableBlock(new RuntimeException(e)).getDataBlock().toBytes()))
- .putMetadata(ChannelUtils.MAILBOX_METADATA_END_OF_STREAM_KEY, "true").build());
+ _errorContent = createErrorContent(e);
Review Comment:
Thanks for catching this. Fixed.
--
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 #9969: [multistage] [bugfix] Throw error when GrpcMailbox receiving buffer is full
Posted by GitBox <gi...@apache.org>.
walterddr merged PR #9969:
URL: https://github.com/apache/pinot/pull/9969
--
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 #9969: [multistage] [bugfix] Throw error when GrpcMailbox receiving buffer is full
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9969:
URL: https://github.com/apache/pinot/pull/9969#discussion_r1046577000
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentStreamObserver.java:
##########
@@ -93,7 +105,19 @@ public void onNext(Mailbox.MailboxContent mailboxContent) {
if (!mailboxContent.getMetadataMap().containsKey(ChannelUtils.MAILBOX_METADATA_BEGIN_OF_STREAM_KEY)) {
// when the receiving end receives a message put it in the mailbox queue.
- _receivingBuffer.offer(mailboxContent);
+ // TODO: pass a timeout to _receivingBuffer.
+ if (!_receivingBuffer.offer(mailboxContent)) {
Review Comment:
question:
1. so when `_receivingBuffer.offer(mailboxContent) == false` this means the buffer somehow cannot be done correctly, yes?
2. do we know what's the timeout behavior is? (is adding a timeout on the `offer`method going to block the onNext() thread and thus block the next GRPC call and eventually creates back pressure? (CC @agavra if you have any thoughts on this)
--
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 #9969: [multistage] [bugfix] Throw error when GrpcMailbox receiving buffer is full
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9969:
URL: https://github.com/apache/pinot/pull/9969#discussion_r1047720806
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentStreamObserver.java:
##########
@@ -93,7 +105,19 @@ public void onNext(Mailbox.MailboxContent mailboxContent) {
if (!mailboxContent.getMetadataMap().containsKey(ChannelUtils.MAILBOX_METADATA_BEGIN_OF_STREAM_KEY)) {
// when the receiving end receives a message put it in the mailbox queue.
- _receivingBuffer.offer(mailboxContent);
+ // TODO: pass a timeout to _receivingBuffer.
+ if (!_receivingBuffer.offer(mailboxContent)) {
Review Comment:
+1
--
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 #9969: [multistage] [bugfix] Throw error when GrpcMailbox receiving buffer is full
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9969:
URL: https://github.com/apache/pinot/pull/9969#issuecomment-1346275477
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9969?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 [#9969](https://codecov.io/gh/apache/pinot/pull/9969?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (420d708) into [master](https://codecov.io/gh/apache/pinot/commit/9c646723403d8a9e2d180ba68ad306489bff48b3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9c64672) will **decrease** coverage by `43.58%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #9969 +/- ##
=============================================
- Coverage 68.70% 25.11% -43.59%
+ Complexity 5061 44 -5017
=============================================
Files 1987 1975 -12
Lines 106768 106426 -342
Branches 16206 16170 -36
=============================================
- Hits 73354 26733 -46621
- Misses 28226 76965 +48739
+ Partials 5188 2728 -2460
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `25.11% <0.00%> (-0.12%)` | :arrow_down: |
| 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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9969?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ache/pinot/query/mailbox/GrpcReceivingMailbox.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9HcnBjUmVjZWl2aW5nTWFpbGJveC5qYXZh) | `0.00% <ø> (-87.50%)` | :arrow_down: |
| [.../mailbox/channel/MailboxContentStreamObserver.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9jaGFubmVsL01haWxib3hDb250ZW50U3RyZWFtT2JzZXJ2ZXIuamF2YQ==) | `0.00% <0.00%> (-70.00%)` | :arrow_down: |
| [...y/mailbox/channel/MailboxStatusStreamObserver.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9jaGFubmVsL01haWxib3hTdGF0dXNTdHJlYW1PYnNlcnZlci5qYXZh) | `0.00% <ø> (-78.27%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/stream/StreamMessage.java](https://codecov.io/gh/apache/pinot/pull/9969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvc3RyZWFtL1N0cmVhbU1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1381 more](https://codecov.io/gh/apache/pinot/pull/9969/diff?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] agavra commented on a diff in pull request #9969: [multistage] [bugfix] Throw error when GrpcMailbox receiving buffer is full
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9969:
URL: https://github.com/apache/pinot/pull/9969#discussion_r1047585319
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentStreamObserver.java:
##########
@@ -93,7 +105,19 @@ public void onNext(Mailbox.MailboxContent mailboxContent) {
if (!mailboxContent.getMetadataMap().containsKey(ChannelUtils.MAILBOX_METADATA_BEGIN_OF_STREAM_KEY)) {
// when the receiving end receives a message put it in the mailbox queue.
- _receivingBuffer.offer(mailboxContent);
+ // TODO: pass a timeout to _receivingBuffer.
+ if (!_receivingBuffer.offer(mailboxContent)) {
Review Comment:
from the documentation:
```
This API is asynchronous, so methods may return before the operation
completes. The API provides no guarantees for how quickly an operation
will complete, so utilizing flow control via ClientCallStreamObserver
and ServerCallStreamObserver to avoid excessive buffering is recommended
for streaming RPCs. gRPC's implementation of onError() on client-side
causes the RPC to be cancelled and discards all messages, so completes
quickly.
gRPC guarantees it does not block on I/O in its implementation,
but applications are allowed to perform blocking operations in
their implementations. However, doing so will delay other callbacks
because the methods cannot be called concurrently.
```
If we block here, I think GRPC will just buffer on its end
--
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] agavra commented on a diff in pull request #9969: [multistage] [bugfix] Throw error when GrpcMailbox receiving buffer is full
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9969:
URL: https://github.com/apache/pinot/pull/9969#discussion_r1047678758
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentStreamObserver.java:
##########
@@ -93,7 +105,19 @@ public void onNext(Mailbox.MailboxContent mailboxContent) {
if (!mailboxContent.getMetadataMap().containsKey(ChannelUtils.MAILBOX_METADATA_BEGIN_OF_STREAM_KEY)) {
// when the receiving end receives a message put it in the mailbox queue.
- _receivingBuffer.offer(mailboxContent);
+ // TODO: pass a timeout to _receivingBuffer.
+ if (!_receivingBuffer.offer(mailboxContent)) {
Review Comment:
yes +1, I agree with this PR
--
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