You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "pradeeee (via GitHub)" <gi...@apache.org> on 2026/05/31 15:40:48 UTC
[PR] Add peer download fallback to PredownloadScheduler [pinot]
pradeeee opened a new pull request, #18641:
URL: https://github.com/apache/pinot/pull/18641
## Problem
The `PredownloadScheduler` runs in a predownload init-container before the main Pinot server starts. It downloads segments from deep storage (e.g., S3, HDFS, GCS) to local disk so the server can start faster.
Currently, if a deep storage download fails (e.g., due to transient network issues, throttling, or deep store unavailability), the segment is marked as failed and the predownload container reports partial failure. There is no fallback mechanism.
In production, deep storage failures can be transient while peer servers holding the same segment are available and healthy. The main Pinot server already supports peer download as a fallback, but the predownload container does not.
## Changes
### PredownloadScheduler.java
1. Constructor now accepts a `boolean peerDownloadEnabled` parameter
2. Reads the peer download scheme from `InstanceDataManagerConfig`
3. Extracted existing deep store download logic into `downloadFromDeepStore()` for clarity
4. Added `downloadFromPeers()` method that:
- Discovers ONLINE peer servers via ExternalView from ZooKeeper
- Shuffles the peer list for load distribution
- Downloads the segment tar file from a peer using `SegmentFetcherFactory.fetchAndDecryptSegmentToLocal()`
- Untars and moves the segment to the final data directory
5. When deep store download fails and peer download is enabled, catches the exception and falls back to peer download
### PredownloadZKClient.java
1. Added `getPeerServerURIs()` method that discovers ONLINE peer servers hosting a given segment
2. Uses ExternalView (not IdealState) to find servers that actually have the segment ready
3. Builds download URIs using the peer's hostname and admin port
4. Mirrors the logic of `PeerServerSegmentFinder` but works without requiring a `HelixManager` instance (which is not available in the predownload container)
### Tests
- `PredownloadSchedulerTest.java`: Added tests for deep store fallback to peer download, peer-only download failure, and constructor with peer download flag
- `PredownloadZKClientTest.java`: Added tests for `getPeerServerURIs()` covering no external view, no online peers, and successful peer discovery
## Test Plan
- Unit tests cover:
- Deep store success (no peer fallback triggered)
- Deep store failure with peer download enabled (fallback to peer)
- Deep store failure with peer download disabled (exception propagated)
- Peer discovery with various ExternalView states
- Verified constructor accepts the new parameter without breaking existing callers
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats merged PR #18641:
URL: https://github.com/apache/pinot/pull/18641
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3464565576
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -336,6 +355,52 @@ void downloadSegment(PredownloadSegmentInfo predownloadSegmentInfo)
}
}
+ private void downloadFromDeepStore(PredownloadSegmentInfo predownloadSegmentInfo)
+ throws Exception {
+ File tempRootDir = getTmpSegmentDataDir(predownloadSegmentInfo);
+ if (_instanceDataManagerConfig.isStreamSegmentDownloadUntar()
+ && predownloadSegmentInfo.getCrypterName() == null) {
+ try {
+ File untaredSegDir = downloadAndStreamUntarWithRateLimit(predownloadSegmentInfo, tempRootDir,
+ _instanceDataManagerConfig.getStreamSegmentDownloadUntarRateLimit());
+ moveSegment(predownloadSegmentInfo, untaredSegDir);
+ } finally {
+ FileUtils.deleteQuietly(tempRootDir);
+ }
+ } else {
+ try {
+ File tarFile = downloadAndDecrypt(predownloadSegmentInfo, tempRootDir);
+ untarAndMoveSegment(predownloadSegmentInfo, tarFile, tempRootDir);
+ } finally {
+ FileUtils.deleteQuietly(tempRootDir);
+ }
+ }
+ }
+
+ private void downloadFromPeers(PredownloadSegmentInfo predownloadSegmentInfo)
Review Comment:
let's add metrics to seggregate calls between deepstore and fallback(peer) download
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "pradeeee (via GitHub)" <gi...@apache.org>.
pradeeee commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3448437476
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadZKClient.java:
##########
@@ -147,6 +149,51 @@ public List<PredownloadSegmentInfo> getSegmentsOfInstance(HelixDataAccessor acce
return predownloadSegmentInfos;
}
+ /**
+ * Returns URIs of ONLINE peer servers hosting the given segment, excluding the current instance.
Review Comment:
The comment is wrong; in the current code, we are taking all the nodes that have a segment hosted and in an online state. This can include the Old node from which replacement is triggered.
The idea is that wherever the segment is healthy, download from there, for some cases where segment is not committed to peer node it will be ava in the old node only.
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3522778049
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadMetrics.java:
##########
@@ -47,6 +47,20 @@ public void segmentDownloaded(boolean succeed, String segmentName, long segmentS
}
}
+ public void peerSegmentDownloaded(boolean succeed, String segmentName, long segmentSizeBytes, long downloadTimeMs) {
+ if (succeed) {
+ _serverMetrics.addMeteredGlobalValue(ServerMeter.PREDOWNLOAD_PEER_SEGMENT_DOWNLOAD_COUNT, 1);
+ _serverMetrics.setValueOfGlobalGauge(ServerGauge.PEER_DOWNLOAD_SPEED,
Review Comment:
Lets add the unit (MBPS) to the metric name to make it explicit.
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18641:
URL: https://github.com/apache/pinot/pull/18641#issuecomment-4881336708
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18641?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 56.97%. Comparing base ([`5a6cd7b`](https://app.codecov.io/gh/apache/pinot/commit/5a6cd7b57094faed9957f1825667bb1b88e1b053?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`aef8d7d`](https://app.codecov.io/gh/apache/pinot/commit/aef8d7dff745726ddf53699a75cadf6bf8af8c5a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 207 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (5a6cd7b) and HEAD (aef8d7d). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (5a6cd7b) | HEAD (aef8d7d) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|integration2|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18641 +/- ##
============================================
- Coverage 64.38% 56.97% -7.42%
+ Complexity 1282 1 -1281
============================================
Files 3359 2603 -756
Lines 207825 153053 -54772
Branches 32447 24917 -7530
============================================
- Hits 133818 87199 -46619
+ Misses 63244 58443 -4801
+ Partials 10763 7411 -3352
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18641/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/18641/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/18641/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/18641/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/18641/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/18641/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.97% <100.00%> (-7.42%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18641/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.97% <100.00%> (-7.42%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18641/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.97% <100.00%> (-7.42%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18641/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.97% <100.00%> (+0.19%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18641/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 Harness](https://app.codecov.io/gh/apache/pinot/pull/18641?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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3522779274
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -107,6 +112,18 @@ public PredownloadScheduler(PropertiesConfiguration properties)
_failedSegments = ConcurrentHashMap.newKeySet();
_executor = Executors.newFixedThreadPool(predownloadParallelism);
LOGGER.info("Created thread pool with num of threads: {}", predownloadParallelism);
+
+ _peerDownloadEnabled = properties.getBoolean("pinot.server.peer.download.enabled", false);
Review Comment:
Can we put `pinot.server.peer.download.enabled` in the defined configs file and use it from there? I think we also have getter methods there - it makes it clearer and reusable.
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3522783422
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadMetrics.java:
##########
@@ -47,6 +47,20 @@ public void segmentDownloaded(boolean succeed, String segmentName, long segmentS
}
}
+ public void peerSegmentDownloaded(boolean succeed, String segmentName, long segmentSizeBytes, long downloadTimeMs) {
+ if (succeed) {
+ _serverMetrics.addMeteredGlobalValue(ServerMeter.PREDOWNLOAD_PEER_SEGMENT_DOWNLOAD_COUNT, 1);
+ _serverMetrics.setValueOfGlobalGauge(ServerGauge.PEER_DOWNLOAD_SPEED,
+ (segmentSizeBytes / BYTES_TO_MB) / (downloadTimeMs / 1000 + 1));
+ } else {
+ _serverMetrics.addMeteredValue(ServerMeter.PREDOWNLOAD_PEER_SEGMENT_DOWNLOAD_FAILURE_COUNT, 1, segmentName);
Review Comment:
Might be very high cardinality if all segments fail to peer download due to a systemic issue. Could we consider sampling this or possibly not having the segmentName in the metric?
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18641:
URL: https://github.com/apache/pinot/pull/18641#issuecomment-4905901911
Opened the docs follow-up PR for this change: https://github.com/pinot-contrib/pinot-docs/pull/901
It documents the new predownload peer-fallback server configs, the node-replacement behavior, and the related monitoring metrics.
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "pradeeee (via GitHub)" <gi...@apache.org>.
pradeeee commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3448484785
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -107,6 +111,11 @@ public PredownloadScheduler(PropertiesConfiguration properties)
_failedSegments = ConcurrentHashMap.newKeySet();
_executor = Executors.newFixedThreadPool(predownloadParallelism);
LOGGER.info("Created thread pool with num of threads: {}", predownloadParallelism);
+
+ _peerDownloadEnabled = peerDownloadEnabled;
+ _peerDownloadScheme = _instanceDataManagerConfig.getSegmentPeerDownloadScheme();
Review Comment:
Done.
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -336,6 +355,52 @@ void downloadSegment(PredownloadSegmentInfo predownloadSegmentInfo)
}
}
+ private void downloadFromDeepStore(PredownloadSegmentInfo predownloadSegmentInfo)
+ throws Exception {
+ File tempRootDir = getTmpSegmentDataDir(predownloadSegmentInfo);
+ if (_instanceDataManagerConfig.isStreamSegmentDownloadUntar()
+ && predownloadSegmentInfo.getCrypterName() == null) {
+ try {
+ File untaredSegDir = downloadAndStreamUntarWithRateLimit(predownloadSegmentInfo, tempRootDir,
+ _instanceDataManagerConfig.getStreamSegmentDownloadUntarRateLimit());
+ moveSegment(predownloadSegmentInfo, untaredSegDir);
+ } finally {
+ FileUtils.deleteQuietly(tempRootDir);
+ }
+ } else {
+ try {
+ File tarFile = downloadAndDecrypt(predownloadSegmentInfo, tempRootDir);
+ untarAndMoveSegment(predownloadSegmentInfo, tarFile, tempRootDir);
+ } finally {
+ FileUtils.deleteQuietly(tempRootDir);
+ }
+ }
+ }
+
+ private void downloadFromPeers(PredownloadSegmentInfo predownloadSegmentInfo)
Review Comment:
Implemented StremUntar flow, it will be preferred, if not enabled then fallback is download -> untar -> move.
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -107,6 +112,18 @@ public PredownloadScheduler(PropertiesConfiguration properties)
_failedSegments = ConcurrentHashMap.newKeySet();
_executor = Executors.newFixedThreadPool(predownloadParallelism);
LOGGER.info("Created thread pool with num of threads: {}", predownloadParallelism);
+
+ _peerDownloadEnabled = properties.getBoolean("pinot.server.peer.download.enabled", false);
Review Comment:
Updated the code, declared inside the configs.
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -336,6 +355,52 @@ void downloadSegment(PredownloadSegmentInfo predownloadSegmentInfo)
}
}
+ private void downloadFromDeepStore(PredownloadSegmentInfo predownloadSegmentInfo)
+ throws Exception {
+ File tempRootDir = getTmpSegmentDataDir(predownloadSegmentInfo);
+ if (_instanceDataManagerConfig.isStreamSegmentDownloadUntar()
+ && predownloadSegmentInfo.getCrypterName() == null) {
+ try {
+ File untaredSegDir = downloadAndStreamUntarWithRateLimit(predownloadSegmentInfo, tempRootDir,
+ _instanceDataManagerConfig.getStreamSegmentDownloadUntarRateLimit());
+ moveSegment(predownloadSegmentInfo, untaredSegDir);
+ } finally {
+ FileUtils.deleteQuietly(tempRootDir);
+ }
+ } else {
+ try {
+ File tarFile = downloadAndDecrypt(predownloadSegmentInfo, tempRootDir);
+ untarAndMoveSegment(predownloadSegmentInfo, tarFile, tempRootDir);
+ } finally {
+ FileUtils.deleteQuietly(tempRootDir);
+ }
+ }
+ }
+
+ private void downloadFromPeers(PredownloadSegmentInfo predownloadSegmentInfo)
Review Comment:
Added below metrics to track the gain from peer download:
PREDOWNLOAD_DEEPSTORE_DOWNLOAD_COUNT - counter for deep store download success
PREDOWNLOAD_PEER_SEGMENT_DOWNLOAD_COUNT - counter for peer download success
PREDOWNLOAD_PEER_SEGMENT_DOWNLOAD_FAILURE_COUNT - counter for peer download failure (with segment name as key)
PEER_DOWNLOAD_SPEED - gauge for peer download speed (MB/s)
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadMetrics.java:
##########
@@ -47,6 +47,20 @@ public void segmentDownloaded(boolean succeed, String segmentName, long segmentS
}
}
+ public void peerSegmentDownloaded(boolean succeed, String segmentName, long segmentSizeBytes, long downloadTimeMs) {
+ if (succeed) {
+ _serverMetrics.addMeteredGlobalValue(ServerMeter.PREDOWNLOAD_PEER_SEGMENT_DOWNLOAD_COUNT, 1);
+ _serverMetrics.setValueOfGlobalGauge(ServerGauge.PEER_DOWNLOAD_SPEED,
Review Comment:
Added the change.
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadMetrics.java:
##########
@@ -47,6 +47,20 @@ public void segmentDownloaded(boolean succeed, String segmentName, long segmentS
}
}
+ public void peerSegmentDownloaded(boolean succeed, String segmentName, long segmentSizeBytes, long downloadTimeMs) {
+ if (succeed) {
+ _serverMetrics.addMeteredGlobalValue(ServerMeter.PREDOWNLOAD_PEER_SEGMENT_DOWNLOAD_COUNT, 1);
+ _serverMetrics.setValueOfGlobalGauge(ServerGauge.PEER_DOWNLOAD_SPEED,
+ (segmentSizeBytes / BYTES_TO_MB) / (downloadTimeMs / 1000 + 1));
+ } else {
+ _serverMetrics.addMeteredValue(ServerMeter.PREDOWNLOAD_PEER_SEGMENT_DOWNLOAD_FAILURE_COUNT, 1, segmentName);
Review Comment:
Dropped it.
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -301,23 +318,25 @@ void downloadSegment(PredownloadSegmentInfo predownloadSegmentInfo)
throws Exception {
try {
long startTime = System.currentTimeMillis();
- File tempRootDir = getTmpSegmentDataDir(predownloadSegmentInfo);
- if (_instanceDataManagerConfig.isStreamSegmentDownloadUntar()
- && predownloadSegmentInfo.getCrypterName() == null) {
- try {
- // TODO: increase rate limit here
- File untaredSegDir = downloadAndStreamUntarWithRateLimit(predownloadSegmentInfo, tempRootDir,
- _instanceDataManagerConfig.getStreamSegmentDownloadUntarRateLimit());
- moveSegment(predownloadSegmentInfo, untaredSegDir);
- } finally {
- FileUtils.deleteQuietly(tempRootDir);
- }
- } else {
- try {
- File tarFile = downloadAndDecrypt(predownloadSegmentInfo, tempRootDir);
- untarAndMoveSegment(predownloadSegmentInfo, tarFile, tempRootDir);
- } finally {
- FileUtils.deleteQuietly(tempRootDir);
+ try {
+ downloadFromDeepStore(predownloadSegmentInfo);
+ _predownloadMetrics.deepStoreSegmentDownloaded();
+ } catch (Exception e) {
+ if (_peerDownloadEnabled && _peerDownloadScheme != null) {
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3440955824
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -107,6 +111,11 @@ public PredownloadScheduler(PropertiesConfiguration properties)
_failedSegments = ConcurrentHashMap.newKeySet();
_executor = Executors.newFixedThreadPool(predownloadParallelism);
LOGGER.info("Created thread pool with num of threads: {}", predownloadParallelism);
+
+ _peerDownloadEnabled = peerDownloadEnabled;
+ _peerDownloadScheme = _instanceDataManagerConfig.getSegmentPeerDownloadScheme();
Review Comment:
nit: Let's sanitize this similar to:
https://github.com/apache/pinot/blob/5526d5b86018bc8d22e10ece356d107dff5e280f/pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java#L242-L247
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadZKClient.java:
##########
@@ -147,6 +149,51 @@ public List<PredownloadSegmentInfo> getSegmentsOfInstance(HelixDataAccessor acce
return predownloadSegmentInfos;
}
+ /**
+ * Returns URIs of ONLINE peer servers hosting the given segment, excluding the current instance.
Review Comment:
nit: "excluding the current instance" This will always be excluded as predownload runs even before server process starts so EV will not have 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
Re: [PR] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "pradeeee (via GitHub)" <gi...@apache.org>.
pradeeee commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3448437476
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadZKClient.java:
##########
@@ -147,6 +149,51 @@ public List<PredownloadSegmentInfo> getSegmentsOfInstance(HelixDataAccessor acce
return predownloadSegmentInfos;
}
+ /**
+ * Returns URIs of ONLINE peer servers hosting the given segment, excluding the current instance.
Review Comment:
The comment is wrong; in the current code, we are taking all the nodes that have a segment hosted and in an online state. This can include the Old node from which replacement is triggered.
The idea is that wherever the segment is healthy, download from there, for some cases where segment is not committed to peer node it will be available in the old node only.
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on PR #18641:
URL: https://github.com/apache/pinot/pull/18641#issuecomment-4785930022
please address the commet, overall LGTM
--
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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3464541273
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -336,6 +355,52 @@ void downloadSegment(PredownloadSegmentInfo predownloadSegmentInfo)
}
}
+ private void downloadFromDeepStore(PredownloadSegmentInfo predownloadSegmentInfo)
+ throws Exception {
+ File tempRootDir = getTmpSegmentDataDir(predownloadSegmentInfo);
+ if (_instanceDataManagerConfig.isStreamSegmentDownloadUntar()
+ && predownloadSegmentInfo.getCrypterName() == null) {
+ try {
+ File untaredSegDir = downloadAndStreamUntarWithRateLimit(predownloadSegmentInfo, tempRootDir,
+ _instanceDataManagerConfig.getStreamSegmentDownloadUntarRateLimit());
+ moveSegment(predownloadSegmentInfo, untaredSegDir);
+ } finally {
+ FileUtils.deleteQuietly(tempRootDir);
+ }
+ } else {
+ try {
+ File tarFile = downloadAndDecrypt(predownloadSegmentInfo, tempRootDir);
+ untarAndMoveSegment(predownloadSegmentInfo, tarFile, tempRootDir);
+ } finally {
+ FileUtils.deleteQuietly(tempRootDir);
+ }
+ }
+ }
+
+ private void downloadFromPeers(PredownloadSegmentInfo predownloadSegmentInfo)
Review Comment:
there are two modes of segment download in deepstoreDownload:
1. download -> untar -> move
2. streaming untar
This method only supports former. Is it possible to handle the second mechanism 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] Add peer download fallback to PredownloadScheduler [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18641:
URL: https://github.com/apache/pinot/pull/18641#discussion_r3522781438
##########
pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java:
##########
@@ -301,23 +318,25 @@ void downloadSegment(PredownloadSegmentInfo predownloadSegmentInfo)
throws Exception {
try {
long startTime = System.currentTimeMillis();
- File tempRootDir = getTmpSegmentDataDir(predownloadSegmentInfo);
- if (_instanceDataManagerConfig.isStreamSegmentDownloadUntar()
- && predownloadSegmentInfo.getCrypterName() == null) {
- try {
- // TODO: increase rate limit here
- File untaredSegDir = downloadAndStreamUntarWithRateLimit(predownloadSegmentInfo, tempRootDir,
- _instanceDataManagerConfig.getStreamSegmentDownloadUntarRateLimit());
- moveSegment(predownloadSegmentInfo, untaredSegDir);
- } finally {
- FileUtils.deleteQuietly(tempRootDir);
- }
- } else {
- try {
- File tarFile = downloadAndDecrypt(predownloadSegmentInfo, tempRootDir);
- untarAndMoveSegment(predownloadSegmentInfo, tarFile, tempRootDir);
- } finally {
- FileUtils.deleteQuietly(tempRootDir);
+ try {
+ downloadFromDeepStore(predownloadSegmentInfo);
+ _predownloadMetrics.deepStoreSegmentDownloaded();
+ } catch (Exception e) {
+ if (_peerDownloadEnabled && _peerDownloadScheme != null) {
Review Comment:
Can we move this logic within `downloadFromPeers` so that this check is always made when `downloadFromPeers` is invoked from some other callsite?
--
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