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 2020/12/21 09:46:14 UTC
[GitHub] [incubator-pinot] samarth-gupta-traceable opened a new pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
samarth-gupta-traceable opened a new pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375
## Description
Fix for https://github.com/apache/incubator-pinot/issues/6374
## Upgrade Notes
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options: yes
new system variable "pinot.server.maxThreadsPerQuery" introduced to configure max threads per query.
## Documentation
If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
----------------------------------------------------------------
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.
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] [incubator-pinot] kishoreg commented on pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#issuecomment-749367187
> Why are we using the JVM system property based mechanism to set this as opposed to using pinot.server.* configurations which is how we set the server instance config?
>
> Also I feel as opposed to making this as a server/instance level config, we should allow this as a table level config or at least both. Will be useful for multi-tenancy and later on implementing query or table level priority. There is a queryConfig field in table config which is used to pass down the table level query timeout information. I think we can enhance QueryConfig to pass down this information as well.
+1 on using per table config
----------------------------------------------------------------
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.
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] [incubator-pinot] samarth-gupta-traceable commented on a change in pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
samarth-gupta-traceable commented on a change in pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#discussion_r551306383
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -236,7 +236,7 @@ public void testRefreshTableConfigAndQueryTimeout()
throws Exception {
// Set timeout as 5ms so that query will timeout
TableConfig tableConfig = getOfflineTableConfig();
- tableConfig.setQueryConfig(new QueryConfig(5L));
+ tableConfig.setQueryConfig(new QueryConfig(5L, 2));
Review comment:
@mayankshriv I am not that familiar with integration tests code. There is no specific reason to use value of 2 here. Pls suggest a reasonable value that can be used here. thanks
----------------------------------------------------------------
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.
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] [incubator-pinot] mcvsubbu commented on pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#issuecomment-749325126
> Why are we using the JVM system property based mechanism to set this as opposed to using pinot.server.* configurations which is how we set the server instance config?
>
> Also I feel as opposed to making this as a server/instance level config, we should allow this as a table level config or at least both. Will be useful for multi-tenancy and later on implementing query or table level priority. There is a queryConfig field in table config which is used to pass down the table level query timeout information. I think we can enhance QueryConfig to pass down this information as well.
+1 to not using jvm property, and use what pinot does with other config items.
Number of threads to use does seem to be a per-system property, though. Even in a multi-tenant system, the table configs will have to carry a weighted value for num threads, which will get hard to provision across tables. I think having this as a per-host config is better.
----------------------------------------------------------------
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.
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] [incubator-pinot] samarth-gupta-traceable commented on a change in pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
samarth-gupta-traceable commented on a change in pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#discussion_r551306383
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -236,7 +236,7 @@ public void testRefreshTableConfigAndQueryTimeout()
throws Exception {
// Set timeout as 5ms so that query will timeout
TableConfig tableConfig = getOfflineTableConfig();
- tableConfig.setQueryConfig(new QueryConfig(5L));
+ tableConfig.setQueryConfig(new QueryConfig(5L, 2));
Review comment:
@mayankshriv I am not that familiar with integration tests code. There is not specific reason to use value of 2 here. Pls suggest a reasonable value that can be used here. thanks
----------------------------------------------------------------
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.
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] [incubator-pinot] kishoreg commented on pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#issuecomment-749366869
> > Why are we using the JVM system property based mechanism to set this as opposed to using pinot.server.* configurations which is how we set the server instance config?
> > Also I feel as opposed to making this as a server/instance level config, we should allow this as a table level config or at least both. Will be useful for multi-tenancy and later on implementing query or table level priority. There is a queryConfig field in table config which is used to pass down the table level query timeout information. I think we can enhance QueryConfig to pass down this information as well.
>
> +1 to not using jvm property, and use what pinot does with other config items.
>
> Number of threads to use does seem to be a per-system property, though. Even in a multi-tenant system, the table configs will have to carry a weighted value for num threads, which will get hard to provision across tables. I think having this as a per-host config is better.
why per host? it's better to control this per table right?
----------------------------------------------------------------
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.
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] [incubator-pinot] samarth-gupta-traceable edited a comment on pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
samarth-gupta-traceable edited a comment on pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#issuecomment-749681512
@siddharthteotia I have made changes to make this part of QueryConfig. However I am not familiar with the code enough and not sure how to pass this along and use in `CombineOperatorUtils` or other place in call hierarchy .
Some pointer or code examples will be helpful. Thanks
cc @kishoreg
----------------------------------------------------------------
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.
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] [incubator-pinot] codecov-io commented on pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#issuecomment-748898114
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6375?src=pr&el=h1) Report
> Merging [#6375](https://codecov.io/gh/apache/incubator-pinot/pull/6375?src=pr&el=desc) (f645b66) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.08%`.
> The diff coverage is `44.92%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6375?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6375 +/- ##
==========================================
- Coverage 66.44% 65.36% -1.09%
==========================================
Files 1075 1294 +219
Lines 54773 62545 +7772
Branches 8168 9091 +923
==========================================
+ Hits 36396 40880 +4484
- Misses 15700 18747 +3047
- Partials 2677 2918 +241
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.36% <44.92%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6375?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
| [...common/config/tuner/NoOpTableTableConfigTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1152 more](https://codecov.io/gh/apache/incubator-pinot/pull/6375/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6375?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6375?src=pr&el=footer). Last update [4183ffe...f645b66](https://codecov.io/gh/apache/incubator-pinot/pull/6375?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-pinot] siddharthteotia commented on pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#issuecomment-749310890
Why are we using the JVM system property based mechanism to set this as opposed to using pinot.server.* configurations which is how we set the server instance config?
Also I feel as opposed to making this as a server/instance level config, we should allow this as a table level config or at least both. Will be useful for multi-tenancy and later on implementing query or table level priority. There is a queryConfig field in table config which is used to pass down the table level query timeout information. I think we can enhance QueryConfig to pass down this information 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.
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] [incubator-pinot] mayankshriv commented on a change in pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#discussion_r547443037
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -236,7 +236,7 @@ public void testRefreshTableConfigAndQueryTimeout()
throws Exception {
// Set timeout as 5ms so that query will timeout
TableConfig tableConfig = getOfflineTableConfig();
- tableConfig.setQueryConfig(new QueryConfig(5L));
+ tableConfig.setQueryConfig(new QueryConfig(5L, 2));
Review comment:
This seems like a change from current behavior. Any reason why we want to constrain the number of cores to be used to 2 for integration tests?
----------------------------------------------------------------
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.
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] [incubator-pinot] samarth-gupta-traceable commented on a change in pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
samarth-gupta-traceable commented on a change in pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#discussion_r551306383
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -236,7 +236,7 @@ public void testRefreshTableConfigAndQueryTimeout()
throws Exception {
// Set timeout as 5ms so that query will timeout
TableConfig tableConfig = getOfflineTableConfig();
- tableConfig.setQueryConfig(new QueryConfig(5L));
+ tableConfig.setQueryConfig(new QueryConfig(5L, 2));
Review comment:
@mayankshriv I am not that familiar with integration tests code. There is no specific reason to use value of 2 here. Pls suggest a reasonable value that can be used here. or I can replace the value with `Runtime.getRuntime().availableProcessors()` thanks
----------------------------------------------------------------
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.
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] [incubator-pinot] samarth-gupta-traceable commented on pull request #6375: make MAX_NUM_THREADS_PER_QUERY configurable for pinto server
Posted by GitBox <gi...@apache.org>.
samarth-gupta-traceable commented on pull request #6375:
URL: https://github.com/apache/incubator-pinot/pull/6375#issuecomment-749681512
@siddharthteotia I have made changes to make this part of QueryConfig. However I am familiar with the code enough and not sure how to pass this along and use in `CombineOperatorUtils` or other place in call hierarchy .
Some pointer or code examples will be helpful. Thanks
cc @kishoreg
----------------------------------------------------------------
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.
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