You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "61yao (via GitHub)" <gi...@apache.org> on 2023/01/24 04:50:15 UTC
[GitHub] [pinot] 61yao opened a new pull request, #10170: [multistage] [refactor] Introduce cancel on error API for MultiStageOperator and sending and receiving mailbox
61yao opened a new pull request, #10170:
URL: https://github.com/apache/pinot/pull/10170
Introduce cancel on error API for MultiStageOperator and mailbox.
This API will be used for MultiStageOperator to signal upstream operator there is an error occurring during execution.
cancel on error API is also introduced for sending and receiving mailbox so we can cancel the corresponding RPC.
waitForFinish API is introduced for sending mailbox because the mailbox send call is async . We need to wait for the RPC calls to be finished to clean up resources.
--
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 #10170: [multistage] [refactor] Introduce cancel on error API for MultiStageOperator and sending and receiving mailbox
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10170:
URL: https://github.com/apache/pinot/pull/10170#discussion_r1088489265
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcReceivingMailbox.java:
##########
@@ -94,6 +91,10 @@ public boolean isClosed() {
return isInitialized() && _contentStreamObserver.isCompleted();
}
+ @Override
+ public void cancel(Throwable e) {
Review Comment:
This PR only introduces API. no actual implementation is 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
[GitHub] [pinot] 61yao commented on a diff in pull request #10170: [multistage] [refactor] Introduce cancel on error API for MultiStageOperator and sending and receiving mailbox
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10170:
URL: https://github.com/apache/pinot/pull/10170#discussion_r1088491354
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SortOperator.java:
##########
@@ -82,6 +82,10 @@ public List<MultiStageOperator> getChildOperators() {
return ImmutableList.of(_upstreamOperator);
}
+ @Override
+ public void cancel(Throwable e) {
+ }
Review Comment:
close() test is covered in OpChainSchedulerServiceTest. cancel is not implemented. so the test will be added in followup PRs when implementation is added.
--
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 #10170: [multistage] [refactor] Introduce cancel on error API for MultiStageOperator and sending and receiving mailbox
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10170:
URL: https://github.com/apache/pinot/pull/10170#issuecomment-1401419711
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10170?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 [#10170](https://codecov.io/gh/apache/pinot/pull/10170?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dfcdd44) into [master](https://codecov.io/gh/apache/pinot/commit/4d7da0abab9563d95d139e11d054da2f0f5d45d8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4d7da0a) will **decrease** coverage by `21.24%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10170 +/- ##
=============================================
- Coverage 34.84% 13.60% -21.24%
+ Complexity 200 176 -24
=============================================
Files 2006 1963 -43
Lines 108718 106521 -2197
Branches 16501 16247 -254
=============================================
- Hits 37880 14496 -23384
- Misses 67566 90884 +23318
+ Partials 3272 1141 -2131
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests2 | `13.60% <0.00%> (-0.04%)` | :arrow_down: |
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/10170?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/GrpcMailboxService.java](https://codecov.io/gh/apache/pinot/pull/10170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9HcnBjTWFpbGJveFNlcnZpY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/query/mailbox/GrpcReceivingMailbox.java](https://codecov.io/gh/apache/pinot/pull/10170?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% <0.00%> (ø)` | |
| [...apache/pinot/query/mailbox/GrpcSendingMailbox.java](https://codecov.io/gh/apache/pinot/pull/10170?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%> (ø)` | |
| [.../pinot/query/mailbox/InMemoryReceivingMailbox.java](https://codecov.io/gh/apache/pinot/pull/10170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9Jbk1lbW9yeVJlY2VpdmluZ01haWxib3guamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...he/pinot/query/mailbox/InMemorySendingMailbox.java](https://codecov.io/gh/apache/pinot/pull/10170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9Jbk1lbW9yeVNlbmRpbmdNYWlsYm94LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../mailbox/channel/MailboxContentStreamObserver.java](https://codecov.io/gh/apache/pinot/pull/10170?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% <ø> (ø)` | |
| [...y/mailbox/channel/MailboxStatusStreamObserver.java](https://codecov.io/gh/apache/pinot/pull/10170?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% <0.00%> (ø)` | |
| [...ot/query/runtime/operator/MailboxSendOperator.java](https://codecov.io/gh/apache/pinot/pull/10170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94U2VuZE9wZXJhdG9yLmphdmE=) | `0.00% <ø> (ø)` | |
| [...not/query/runtime/operator/MultiStageOperator.java](https://codecov.io/gh/apache/pinot/pull/10170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NdWx0aVN0YWdlT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...g/apache/pinot/query/runtime/operator/OpChain.java](https://codecov.io/gh/apache/pinot/pull/10170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9PcENoYWluLmphdmE=) | `0.00% <0.00%> (ø)` | |
| ... and [785 more](https://codecov.io/gh/apache/pinot/pull/10170?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] 61yao commented on pull request #10170: [multistage] [refactor] Introduce cancel on error API for MultiStageOperator and sending and receiving mailbox
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on PR #10170:
URL: https://github.com/apache/pinot/pull/10170#issuecomment-1403155747
@xiangfu0
--
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] siddharthteotia merged pull request #10170: [multistage] [refactor] Introduce cancel on error API for MultiStageOperator and sending and receiving mailbox
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia merged PR #10170:
URL: https://github.com/apache/pinot/pull/10170
--
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 #10170: [multistage] [refactor] Introduce cancel on error API for MultiStageOperator and sending and receiving mailbox
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10170:
URL: https://github.com/apache/pinot/pull/10170#discussion_r1088489780
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/SendingMailbox.java:
##########
@@ -46,4 +49,8 @@ void send(T data)
* Complete delivery of the current mailbox.
*/
void complete();
+
+ void waitForFinish(long timeout, TimeUnit unit) throws InterruptedException;
Review Comment:
Yeah. will hook the cancel and waitForFinish in followup PRs
--
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 #10170: [multistage] [refactor] Introduce cancel on error API for MultiStageOperator and sending and receiving mailbox
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10170:
URL: https://github.com/apache/pinot/pull/10170#discussion_r1088445842
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SortOperator.java:
##########
@@ -82,6 +82,10 @@ public List<MultiStageOperator> getChildOperators() {
return ImmutableList.of(_upstreamOperator);
}
+ @Override
+ public void cancel(Throwable e) {
+ }
Review Comment:
could you also add tests to cover the overwritten cancel and close methods?
for example this one seems like if we encountered a SORT at the top it will not populate cancellation downwards?
--
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 #10170: [multistage] [refactor] Introduce cancel on error API for MultiStageOperator and sending and receiving mailbox
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10170:
URL: https://github.com/apache/pinot/pull/10170#discussion_r1088441421
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcReceivingMailbox.java:
##########
@@ -94,6 +91,10 @@ public boolean isClosed() {
return isInitialized() && _contentStreamObserver.isCompleted();
}
+ @Override
+ public void cancel(Throwable e) {
Review Comment:
shouldn't we clean up the `_contentStreamObserver` in this method?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OpChain.java:
##########
@@ -66,4 +66,8 @@ public String toString() {
public void close() {
_root.close();
}
+
+ public void cancel(Throwable e) {
Review Comment:
please add javadoc explaining the difference between cancel and close API
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxStatusStreamObserver.java:
##########
@@ -30,31 +30,18 @@
* {@code MailboxStatusStreamObserver} is used by the SendingMailbox to send data over the wire.
*
* <p>Once {@link org.apache.pinot.query.mailbox.GrpcSendingMailbox#init()} is called, one instances of this class is
- * created based on the opened GRPC connection returned {@link StreamObserver}. From this point, the sending mailbox
- * can use the {@link MailboxStatusStreamObserver#send(Mailbox.MailboxContent)} API to send data packet to the receiving
+ * created based on the opened GRPC connection returned {@link StreamObserver}.
* end.
*/
public class MailboxStatusStreamObserver implements StreamObserver<Mailbox.MailboxStatus> {
private static final Logger LOGGER = LoggerFactory.getLogger(MailboxStatusStreamObserver.class);
private static final int DEFAULT_MAILBOX_QUEUE_CAPACITY = 5;
private final AtomicInteger _bufferSize = new AtomicInteger(5);
- private final AtomicBoolean _isCompleted = new AtomicBoolean(false);
- private StreamObserver<Mailbox.MailboxContent> _mailboxContentStreamObserver;
+ private CountDownLatch _finishLatch;
- public MailboxStatusStreamObserver() {
- }
-
- public void init(StreamObserver<Mailbox.MailboxContent> mailboxContentStreamObserver) {
- _mailboxContentStreamObserver = mailboxContentStreamObserver;
- }
-
- public void send(Mailbox.MailboxContent mailboxContent) {
- _mailboxContentStreamObserver.onNext(mailboxContent);
- }
-
- public void complete() {
- _mailboxContentStreamObserver.onCompleted();
Review Comment:
nice refactoring. i dont think we need these wrapper anytime soon.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/SendingMailbox.java:
##########
@@ -46,4 +49,8 @@ void send(T data)
* Complete delivery of the current mailbox.
*/
void complete();
+
+ void waitForFinish(long timeout, TimeUnit unit) throws InterruptedException;
Review Comment:
i dont see `waitForFinish` being called in any place. is it 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