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/05/12 23:32:24 UTC
[GitHub] [pinot] jackjlli opened a new pull request, #8694: Set max number of parallel segment downloads per table in pinot-server
jackjlli opened a new pull request, #8694:
URL: https://github.com/apache/pinot/pull/8694
This PR adds the ability to set the max number of parallel segment downloads per table in pinot-server. There are a couple of reasons:
* Currently there is no upper limit on how many segment download requests that server can send to controllers. In some cases like table rebalance, if the number of download requests is high, this burst of download requests will affect the availability of pinot controllers to serve other requests.
* By default there is no timeout on helix state transition. As long as the transition hasn't finished, the segment won't be online for serving queries. Thus, there is no side effect on querying part.
--
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] jackjlli merged pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
jackjlli merged PR #8694:
URL: https://github.com/apache/pinot/pull/8694
--
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] jadami10 commented on pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
jadami10 commented on PR #8694:
URL: https://github.com/apache/pinot/pull/8694#issuecomment-1131893840
For my understanding, does this feature implicitly lower impact on the servers as well? If we set this and the controller is using the semaphore, does this mean the servers will also be throttled in the rate they download?
For context, I just pushed 4096 segments to a table, and the servers were at 100% CPU for ~30 minutes while it downloaded the segments and finished building indices.
--
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] jackjlli commented on pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
jackjlli commented on PR #8694:
URL: https://github.com/apache/pinot/pull/8694#issuecomment-1131967348
> For my understanding, does this feature implicitly lower impact on the servers as well? If we set this and the controller is using the semaphore, does this mean the servers will also be throttled in the rate they download?
>
> For context, I just pushed 4096 segments to a table, and the servers were at 100% CPU for ~30 minutes while it downloaded the segments and finished building indices.
@jadami10 Yes, this could be one of the optimizations to lower your CPU usage. While as you mentioned there are two major steps that require servers to handle. For building indices, you can enable the index generation logic during the segment generation so that servers won't be busy doing so. You can enable it in the table config. Take a look at this:
https://github.com/apache/pinot/blob/master/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentGeneratorConfig.java#L181
--
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 commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r871985051
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -72,6 +73,8 @@ public abstract class BaseTableDataManager implements TableDataManager {
private static final Logger LOGGER = LoggerFactory.getLogger(BaseTableDataManager.class);
protected final ConcurrentHashMap<String, SegmentDataManager> _segmentDataManagerMap = new ConcurrentHashMap<>();
+ // Semaphore to restrict the maximum number of parallel segment downloads for a table.
+ private Semaphore _segmentDownloadSemaphore;
Review Comment:
My suggestion was not to reuse the same `object/instance` (which will be problematic like you mentioned) but the implementation instead and instantiate it here with the required number of permits for download because generically, semaphore just needs those 2 configs and can be instantiated separately by each different user for different purpose.
But this is not super necessary. The current approach is fine
--
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] jackjlli commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r871999928
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManagerConfig.java:
##########
@@ -74,6 +74,13 @@ public class HelixInstanceDataManagerConfig implements InstanceDataManagerConfig
private static final String MAX_PARALLEL_SEGMENT_BUILDS = "realtime.max.parallel.segment.builds";
private static final int DEFAULT_MAX_PARALLEL_SEGMENT_BUILDS = 4;
+ // Key of how many parallel segment downloads can be made per table.
+ // A value of <= 0 indicates unlimited.
+ // Unlimited parallel downloads can make Pinot controllers receive high burst of download requests,
+ // causing controllers unavailable for that period of time.
+ private static final String MAX_PARALLEL_SEGMENT_DOWNLOADS = "table.level.max.parallel.segment.downloads";
+ private static final int DEFAULT_MAX_PARALLEL_SEGMENT_DOWNLOADS = 4;
Review Comment:
Updated it to -1 by default, which is disabling it by default.
--
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 commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r871940730
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -403,6 +411,9 @@ File downloadAndDecrypt(String segmentName, SegmentZKMetadata zkMetadata, File t
File tarFile = new File(tempRootDir, segmentName + TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
String uri = zkMetadata.getDownloadUrl();
try {
+ if (_segmentDownloadSemaphore != null) {
+ _segmentDownloadSemaphore.acquire();
Review Comment:
We should add info log for acquire
--
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] jadami10 commented on pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
jadami10 commented on PR #8694:
URL: https://github.com/apache/pinot/pull/8694#issuecomment-1132047513
> @jadami10 Yes, this could be one of the optimizations to lower your CPU usage
that's great! I'll give this a try next week
> For building indices, you can enable the index generation logic during the segment generation so that servers won't be busy doing so. You can enable it in the table config. Take a look at this:
unfortunately it's building a range index and a bloom filter which don't seem to be configurable to be done at segment generation
--
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 commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r871940339
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -72,6 +73,8 @@ public abstract class BaseTableDataManager implements TableDataManager {
private static final Logger LOGGER = LoggerFactory.getLogger(BaseTableDataManager.class);
protected final ConcurrentHashMap<String, SegmentDataManager> _segmentDataManagerMap = new ConcurrentHashMap<>();
+ // Semaphore to restrict the maximum number of parallel segment downloads for a table.
+ private Semaphore _segmentDownloadSemaphore;
Review Comment:
I suggest doing something similar to `SegmentRefreshSemaphore` by wrapping the Semaphore inside it or can we just reuse it here since it is reused for both refresh and reload ? May have to change the log message to be more appropriate
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -119,6 +122,11 @@ public void init(TableDataManagerConfig tableDataManagerConfig, String instanceI
_resourceTmpDir);
}
_errorCache = errorCache;
+ if (maxParallelSegmentDownloads > 0) {
Review Comment:
We should add info log for acquire
--
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 commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r871985051
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -72,6 +73,8 @@ public abstract class BaseTableDataManager implements TableDataManager {
private static final Logger LOGGER = LoggerFactory.getLogger(BaseTableDataManager.class);
protected final ConcurrentHashMap<String, SegmentDataManager> _segmentDataManagerMap = new ConcurrentHashMap<>();
+ // Semaphore to restrict the maximum number of parallel segment downloads for a table.
+ private Semaphore _segmentDownloadSemaphore;
Review Comment:
My suggestion was not to reuse the same `object/instance` (which will be problematic like you mentioned0 but the implementation instead and instantiate it here with the required number of permits for download because generically, semaphore just needs those 2 configs and can be instantiated separately by each different user for different purpose.
But this is not super necessary. The current approach is fine
--
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] jackjlli commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r871979447
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -403,6 +411,9 @@ File downloadAndDecrypt(String segmentName, SegmentZKMetadata zkMetadata, File t
File tarFile = new File(tempRootDir, segmentName + TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
String uri = zkMetadata.getDownloadUrl();
try {
+ if (_segmentDownloadSemaphore != null) {
+ _segmentDownloadSemaphore.acquire();
Review Comment:
Info log 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 #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8694:
URL: https://github.com/apache/pinot/pull/8694#issuecomment-1125530606
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8694?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 [#8694](https://codecov.io/gh/apache/pinot/pull/8694?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8a5cba) into [master](https://codecov.io/gh/apache/pinot/commit/088da3f8c2f7077c51b7d8531b7b967ad1cf58c6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (088da3f) will **decrease** coverage by `51.82%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head e8a5cba differs from pull request most recent head b915f2e. Consider uploading reports for the commit b915f2e to get more accurate results
```diff
@@ Coverage Diff @@
## master #8694 +/- ##
=============================================
- Coverage 66.16% 14.34% -51.83%
+ Complexity 4387 168 -4219
=============================================
Files 1277 1677 +400
Lines 64430 87922 +23492
Branches 10014 13115 +3101
=============================================
- Hits 42633 12614 -30019
- Misses 18786 74374 +55588
+ Partials 3011 934 -2077
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `?` | |
| unittests2 | `14.34% <0.00%> (?)` | |
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/8694?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/8694/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-86.39%)` | :arrow_down: |
| [...data/manager/offline/TableDataManagerProvider.java](https://codecov.io/gh/apache/pinot/pull/8694/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvb2ZmbGluZS9UYWJsZURhdGFNYW5hZ2VyUHJvdmlkZXIuamF2YQ==) | `0.00% <0.00%> (-64.29%)` | :arrow_down: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8694/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8694/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8694/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8694/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8694/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8694/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8694/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/8694/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: |
| ... and [1515 more](https://codecov.io/gh/apache/pinot/pull/8694/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8694?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8694?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [088da3f...b915f2e](https://codecov.io/gh/apache/pinot/pull/8694?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] siddharthteotia commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r871987948
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManagerConfig.java:
##########
@@ -74,6 +74,13 @@ public class HelixInstanceDataManagerConfig implements InstanceDataManagerConfig
private static final String MAX_PARALLEL_SEGMENT_BUILDS = "realtime.max.parallel.segment.builds";
private static final int DEFAULT_MAX_PARALLEL_SEGMENT_BUILDS = 4;
+ // Key of how many parallel segment downloads can be made per table.
+ // A value of <= 0 indicates unlimited.
+ // Unlimited parallel downloads can make Pinot controllers receive high burst of download requests,
+ // causing controllers unavailable for that period of time.
+ private static final String MAX_PARALLEL_SEGMENT_DOWNLOADS = "table.level.max.parallel.segment.downloads";
+ private static final int DEFAULT_MAX_PARALLEL_SEGMENT_DOWNLOADS = 4;
Review Comment:
let's keep the default behavior as is and instead internally configure what we need ?
--
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] jackjlli commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r872619024
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -72,6 +73,8 @@ public abstract class BaseTableDataManager implements TableDataManager {
private static final Logger LOGGER = LoggerFactory.getLogger(BaseTableDataManager.class);
protected final ConcurrentHashMap<String, SegmentDataManager> _segmentDataManagerMap = new ConcurrentHashMap<>();
+ // Semaphore to restrict the maximum number of parallel segment downloads for a table.
+ private Semaphore _segmentDownloadSemaphore;
Review Comment:
Agreed. Later on we can improve it to be dynamic instead of a server level 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.
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 commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r871940150
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -119,6 +122,11 @@ public void init(TableDataManagerConfig tableDataManagerConfig, String instanceI
_resourceTmpDir);
}
_errorCache = errorCache;
+ if (maxParallelSegmentDownloads > 0) {
Review Comment:
We should add info log for acquire
--
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] jackjlli commented on a diff in pull request #8694: Set max number of parallel segment downloads per table in pinot-server
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8694:
URL: https://github.com/apache/pinot/pull/8694#discussion_r871976850
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -72,6 +73,8 @@ public abstract class BaseTableDataManager implements TableDataManager {
private static final Logger LOGGER = LoggerFactory.getLogger(BaseTableDataManager.class);
protected final ConcurrentHashMap<String, SegmentDataManager> _segmentDataManagerMap = new ConcurrentHashMap<>();
+ // Semaphore to restrict the maximum number of parallel segment downloads for a table.
+ private Semaphore _segmentDownloadSemaphore;
Review Comment:
Good question! Let me explain a bit on why I didn't choose to use the same existing semaphore in this PR:
* For segment reload, it doesn't have to always download the segment. If the same semaphore is reused, pinot server could lose the ability to download for an actual download request while it's blocked by the same semaphore which is used for reloading a local segment.
* The default number of segment to be refresh/reload is 1, meaning that for reloading all segments request, there is at most 1 segment to be reloaded, which is also different from the download request.
* For segment refresh/reload, the target segments are already shown in external view, which means that these segments are already used for querying. While download requests could be made by just adding a segment or table rebalance, during which the EV could not have been converged yet. Thus, it's okay to have higher parallelism for those cases.
--
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