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/06/06 17:29:36 UTC
[GitHub] [pinot] walterddr opened a new pull request, #10858: [hotfix] use proper threadpool for test
walterddr opened a new pull request, #10858:
URL: https://github.com/apache/pinot/pull/10858
we upgraded the intermediate stage threadpool to cached threadpool already. but forgot to change the one in test.
--
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] klsince commented on pull request #10858: [hotfix] use proper threadpool for test
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on PR #10858:
URL: https://github.com/apache/pinot/pull/10858#issuecomment-1579204387
> ... thus all threads within the threadpool are blocked and no more tasks can be scheduled into it
so basically, there are dependencies between tasks from different stages, and a fixed thread pool might have led to dead lock among them, like those taking threads and running might actually wait for tasks waiting in the queue to complete.
--
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] ege-st commented on pull request #10858: [hotfix] use proper threadpool for test
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #10858:
URL: https://github.com/apache/pinot/pull/10858#issuecomment-1579184306
Why did the unit tests not using cached thread pool lead to the tests occasionally failing?
--
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 #10858: [hotfix] use proper threadpool for test
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10858:
URL: https://github.com/apache/pinot/pull/10858#discussion_r1220061034
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/QueryServerEnclosure.java:
##########
@@ -69,7 +68,7 @@ public class QueryServerEnclosure {
private final HelixManager _helixManager;
private final QueryRunner _queryRunner;
- private final ExecutorService _executor = Executors.newFixedThreadPool(ResourceManager.DEFAULT_QUERY_RUNNER_THREADS,
+ private final ExecutorService _executor = Executors.newCachedThreadPool(
Review Comment:
technically only this one needs to be changed to cached threadpool. happy to only make this change if that makes more sense
--
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 #10858: [hotfix] use proper threadpool for test
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10858:
URL: https://github.com/apache/pinot/pull/10858#issuecomment-1579284373
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10858?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10858](https://app.codecov.io/gh/apache/pinot/pull/10858?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9035505) into [master](https://app.codecov.io/gh/apache/pinot/commit/84688577e40ef34dcb2aadf558ec759ca80466da?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8468857) will **decrease** coverage by `7.11%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #10858 +/- ##
============================================
- Coverage 70.23% 63.12% -7.11%
+ Complexity 6585 5283 -1302
============================================
Files 2170 2158 -12
Lines 116665 116404 -261
Branches 17656 17633 -23
============================================
- Hits 81942 73485 -8457
- Misses 29014 37539 +8525
+ Partials 5709 5380 -329
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `23.93% <ø> (-0.10%)` | :arrow_down: |
| integration2 | `23.71% <ø> (+0.11%)` | :arrow_up: |
| unittests1 | `67.76% <ø> (+0.02%)` | :arrow_up: |
| 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.
[see 314 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10858/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 commented on pull request #10858: [hotfix] use proper threadpool for test
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10858:
URL: https://github.com/apache/pinot/pull/10858#issuecomment-1579278441
actually it doesn't really matter b/c the test using the 2 worker/runner theadpool is not important. will land.
--
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 #10858: [hotfix] use proper threadpool for test
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10858:
URL: https://github.com/apache/pinot/pull/10858#issuecomment-1579274631
> so basically, there are dependencies between tasks from different stages, and a fixed thread pool might have led to dead lock among them, like those taking threads and running might actually wait for tasks waiting in the queue to complete.
partially correct
- the worker/runner thread are deadlock/starvation free, so using a fixed threadpool would not have any issues
- i modified them in this PR but it is not necessary. in fact in incline to revert them b/c it help catch some distributed deadlock in workers.
- the server thread which handles compiling the query is not deadlock-free, and it is designed specifically to be blocked there.
- which is the one that fixes the flaky test in this PR (the one I commented: https://github.com/apache/pinot/pull/10858#discussion_r1220061034)
--
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 #10858: [hotfix] use proper threadpool for test
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10858:
URL: https://github.com/apache/pinot/pull/10858#issuecomment-1579187444
> Why did the unit tests not using cached thread pool lead to the tests occasionally failing?
GHA servers only has 2 virtual core, and in the pipeline breaker scenario we will have more stages deployed (waiting for pipeline breaker to finish) than the number of cores on the fly, thus all threads within the threadpool are blocked and no more tasks can be scheduled into it
--
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 merged pull request #10858: [hotfix] use proper threadpool for test
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10858:
URL: https://github.com/apache/pinot/pull/10858
--
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