You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2025/10/13 09:07:17 UTC
[PR] Remove non splitter grpc sender [pinot]
gortiz opened a new pull request, #17003:
URL: https://github.com/apache/pinot/pull/17003
In https://github.com/apache/pinot/pull/16999, I tried to improve the error message when queries fail when trying to send gRPC messages larger than max inbound size.
As suggested by @Jackie-Jiang, this PR follows a different approach: instead of improving the message, it changes the code to always use the splitter sender.
--
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
Re: [PR] Remove non splitter grpc sender [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #17003:
URL: https://github.com/apache/pinot/pull/17003
--
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
Re: [PR] Remove non splitter grpc sender [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17003:
URL: https://github.com/apache/pinot/pull/17003#issuecomment-3405482330
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17003?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `95.23810%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.28%. Comparing base ([`eb041ce`](https://app.codecov.io/gh/apache/pinot/commit/eb041ce45c65f1355ef3d95899de2e86143502d6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`446756c`](https://app.codecov.io/gh/apache/pinot/commit/446756c4f81d3b494c20ef9e2079a3879939c17d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17003?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../pinot/query/service/dispatch/QueryDispatcher.java](https://app.codecov.io/gh/apache/pinot/pull/17003?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fservice%2Fdispatch%2FQueryDispatcher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9kaXNwYXRjaC9RdWVyeURpc3BhdGNoZXIuamF2YQ==) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17003?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (eb041ce) and HEAD (446756c). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (eb041ce) | HEAD (446756c) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|3|1|
>|temurin|9|1|
>|java-11|4|0|
>|unittests2|1|0|
>|integration|6|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17003 +/- ##
============================================
- Coverage 63.49% 56.28% -7.21%
+ Complexity 1419 701 -718
============================================
Files 3081 2416 -665
Lines 181851 134753 -47098
Branches 27907 21522 -6385
============================================
- Hits 115464 75847 -39617
+ Misses 57502 52576 -4926
+ Partials 8885 6330 -2555
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.28% <95.23%> (-7.19%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.28% <95.23%> (-7.21%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.28% <95.23%> (-7.21%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.28% <95.23%> (-0.04%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17003?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] Remove non splitter grpc sender [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17003:
URL: https://github.com/apache/pinot/pull/17003#discussion_r2425646757
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -168,6 +168,7 @@ public QueryResult submitAndReduce(RequestContext context, DispatchableSubPlan d
submit(requestId, dispatchableSubPlan, timeoutMs, servers, queryOptions);
QueryResult result = runReducer(dispatchableSubPlan, queryOptions, _mailboxService);
if (result.getProcessingException() != null) {
+ LOGGER.warn("Query result includes processing exceptions. Trying to cancel the other opchains");
Review Comment:
The term 'opchains' appears to be a typo and should likely be 'op chains' or 'operation chains'.
```suggestion
LOGGER.warn("Query result includes processing exceptions. Trying to cancel the other op chains");
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1892,7 +1892,11 @@ public static class RewriterConstants {
*/
public static class MultiStageQueryRunner {
/**
- * Configuration for mailbox data block size
+ * Configuration for mailbox data block size.
+ *
+ * Ideally it should be in the order of a few MBs, to balance the serialization/deserialization overhead and the
+ * number of messages to transfer. Values lower tha hundreds of KBs are not recommended and may lead to excessive
Review Comment:
Corrected spelling of 'tha' to 'than'.
```suggestion
* number of messages to transfer. Values lower than hundreds of KBs are not recommended and may lead to excessive
```
--
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
Re: [PR] Remove non splitter grpc sender [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17003:
URL: https://github.com/apache/pinot/pull/17003#discussion_r2435031743
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcSendingMailbox.java:
##########
@@ -68,19 +67,19 @@ public class GrpcSendingMailbox implements SendingMailbox {
private final long _deadlineMs;
private final StatMap<MailboxSendOperator.StatKey> _statMap;
private final MailboxStatusObserver _statusObserver = new MailboxStatusObserver();
- private final Sender _sender;
+ private final int _maxByteStringSize;
private StreamObserver<MailboxContent> _contentObserver;
public GrpcSendingMailbox(String id, ChannelManager channelManager, String hostname, int port, long deadlineMs,
- StatMap<MailboxSendOperator.StatKey> statMap, int maxByteStringSize) {
+ StatMap<MailboxSendOperator.StatKey> statMap, int maxInboundMessageSize) {
_id = id;
_channelManager = channelManager;
_hostname = hostname;
_port = port;
_deadlineMs = deadlineMs;
_statMap = statMap;
- _sender = maxByteStringSize > 0 ? new SplitSender(this, maxByteStringSize) : new NonSplitSender(this);
+ _maxByteStringSize = Math.max(maxInboundMessageSize / 2, 1);
Review Comment:
Done
--
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
Re: [PR] Remove non splitter grpc sender [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17003:
URL: https://github.com/apache/pinot/pull/17003#discussion_r2435021829
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/mailbox/MailboxServiceTest.java:
##########
@@ -80,7 +79,7 @@ public void setUpStats() {
_stats = new StatMap<>(MailboxSendOperator.StatKey.class);
}
- @Test
+ @Test(timeOut = 100000)
Review Comment:
Removed
--
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
Re: [PR] Remove non splitter grpc sender [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17003:
URL: https://github.com/apache/pinot/pull/17003#discussion_r2435244664
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentObserver.java:
##########
@@ -143,8 +143,8 @@ public void onError(Throwable t) {
_mailboxBuffers.clear();
if (_mailbox != null) {
String msg = t != null ? t.getMessage() : "Unknown";
- _mailbox.setErrorBlock(ErrorMseBlock.fromError(
- QueryErrorCode.QUERY_CANCELLATION, "Cancelled by sender with exception: " + msg), List.of());
+ String errorMessage = "GRPC mailbox cancelled by sender with exception: " + msg;
+ _mailbox.setErrorBlock(ErrorMseBlock.fromError(QueryErrorCode.QUERY_CANCELLATION, errorMessage), List.of());
Review Comment:
Done as well
--
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
Re: [PR] Remove non splitter grpc sender [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17003:
URL: https://github.com/apache/pinot/pull/17003#discussion_r2427030593
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/mailbox/MailboxServiceTest.java:
##########
@@ -80,7 +79,7 @@ public void setUpStats() {
_stats = new StatMap<>(MailboxSendOperator.StatKey.class);
}
- @Test
+ @Test(timeOut = 100000)
Review Comment:
My changes broke one of the tests, which never reached the termination condition. As a result, GHA job was running for 3 hours and the log wasn't very useful to understand what was going on. We should use the await version that accepts a timeout (ie I changed some tests to use receiveMailLatch.await(10000, TimeUnit.MILLISECONS)) but anyway having a timeout on tests would be great.
Before changing all this @Test, I asked chatgpt for a way to set a default timeout for testng, but it wasn't able to tell me how. I guess it is another reason to eventually move to JUnit 5 (or even 6, which was recently released!)
--
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
Re: [PR] Remove non splitter grpc sender [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17003:
URL: https://github.com/apache/pinot/pull/17003#discussion_r2427039637
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/mailbox/MailboxServiceTest.java:
##########
@@ -80,7 +79,7 @@ public void setUpStats() {
_stats = new StatMap<>(MailboxSendOperator.StatKey.class);
}
- @Test
+ @Test(timeOut = 100000)
Review Comment:
Agree we should definitely use conditioned wait.
Seems most of the tests are already using conditioned wait, and this timeout is not needed
--
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
Re: [PR] Remove non splitter grpc sender [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17003:
URL: https://github.com/apache/pinot/pull/17003#discussion_r2426977054
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentObserver.java:
##########
@@ -143,8 +143,8 @@ public void onError(Throwable t) {
_mailboxBuffers.clear();
if (_mailbox != null) {
String msg = t != null ? t.getMessage() : "Unknown";
- _mailbox.setErrorBlock(ErrorMseBlock.fromError(
- QueryErrorCode.QUERY_CANCELLATION, "Cancelled by sender with exception: " + msg), List.of());
+ String errorMessage = "GRPC mailbox cancelled by sender with exception: " + msg;
+ _mailbox.setErrorBlock(ErrorMseBlock.fromError(QueryErrorCode.QUERY_CANCELLATION, errorMessage), List.of());
Review Comment:
Not introduced by this PR, but I feel this shouldn't be count as `QUERY_CANCELLATION`. It is probably an `INTERNAL` error
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/mailbox/MailboxServiceTest.java:
##########
@@ -80,7 +79,7 @@ public void setUpStats() {
_stats = new StatMap<>(MailboxSendOperator.StatKey.class);
}
- @Test
+ @Test(timeOut = 100000)
Review Comment:
Is it recommended to use `timeOut` here? Is it possible to make it on the class level so that we don't need to put it for every method?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcSendingMailbox.java:
##########
@@ -68,19 +67,19 @@ public class GrpcSendingMailbox implements SendingMailbox {
private final long _deadlineMs;
private final StatMap<MailboxSendOperator.StatKey> _statMap;
private final MailboxStatusObserver _statusObserver = new MailboxStatusObserver();
- private final Sender _sender;
+ private final int _maxByteStringSize;
private StreamObserver<MailboxContent> _contentObserver;
public GrpcSendingMailbox(String id, ChannelManager channelManager, String hostname, int port, long deadlineMs,
- StatMap<MailboxSendOperator.StatKey> statMap, int maxByteStringSize) {
+ StatMap<MailboxSendOperator.StatKey> statMap, int maxInboundMessageSize) {
_id = id;
_channelManager = channelManager;
_hostname = hostname;
_port = port;
_deadlineMs = deadlineMs;
_statMap = statMap;
- _sender = maxByteStringSize > 0 ? new SplitSender(this, maxByteStringSize) : new NonSplitSender(this);
+ _maxByteStringSize = Math.max(maxInboundMessageSize / 2, 1);
Review Comment:
Consider adding a TODO to revisit this decision. If there are certain overhead (e.g. bytes added to the serialized data) for gRPC message, we should leave absolute bytes buffer, instead of only allowing half size
--
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