You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "ankitsultana (via GitHub)" <gi...@apache.org> on 2023/05/09 22:12:28 UTC
[GitHub] [pinot] ankitsultana opened a new pull request, #10752: [Draft] Use Non-Random Path in Pinot Deepstore Upload Retry
ankitsultana opened a new pull request, #10752:
URL: https://github.com/apache/pinot/pull/10752
TBD
--
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 a diff in pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1196870494
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1555,4 +1562,19 @@ public PauseStatus getPauseStatus(String tableNameWithType) {
Set<String> consumingSegments = findConsumingSegments(idealState);
return new PauseStatus(Boolean.parseBoolean(isTablePausedStr), consumingSegments, null);
}
+
+ @VisibleForTesting
+ String moveSegmentFile(String rawTableName, String segmentName, String segmentLocation, PinotFS pinotFS)
+ throws IOException {
+ URI segmentFileURI = URIUtils.getUri(segmentLocation);
+ URI uriToMoveTo = createSegmentPath(rawTableName, segmentName);
Review Comment:
is there need to compare the two URIs and bail out if they are same? or we assume pinotFS.move() does the right thing. On a quick check of `BasePinotFS.move()`, it deletes `dstUri` before moving, so if the two URIs are same, then we lose the src too.
--
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] chenboat merged pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat merged PR #10752:
URL: https://github.com/apache/pinot/pull/10752
--
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] chenboat commented on a diff in pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1195835315
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentUploader.java:
##########
@@ -25,5 +25,15 @@
public interface SegmentUploader {
// Returns the URI of the uploaded segment. null if the upload fails.
+
+ /**
+ * Uploads the given segmentFile to the deep-store. The upload to deep-store involve calling another Pinot component.
+ */
Review Comment:
Can you combine the javadoc in line 27 in the new comment too?
--
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] ankitsultana commented on a diff in pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1192543509
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1438,9 +1441,13 @@ public void uploadToDeepStoreIfMissing(TableConfig tableConfig, List<SegmentZKMe
// Randomly ask one server to upload
URI uri = peerSegmentURIs.get(RANDOM.nextInt(peerSegmentURIs.size()));
String serverUploadRequestUrl = StringUtil.join("/", uri.toString(), "upload");
+ serverUploadRequestUrl =
+ String.format("%s?uploadTimeoutMs=%d", serverUploadRequestUrl, _deepstoreUploadRetryTimeoutMs);
LOGGER.info("Ask server to upload LLC segment {} to deep store by this path: {}", segmentName,
serverUploadRequestUrl);
- String segmentDownloadUrl = _fileUploadDownloadClient.uploadToSegmentStore(serverUploadRequestUrl);
+ String tempSegmentDownloadUrl = _fileUploadDownloadClient.uploadToSegmentStore(serverUploadRequestUrl);
Review Comment:
Just a note: the http-connection timeout used here is 10 minutes. For all practical purposes, this should work fine.
@walterddr : lmk if we should set this to be equal to `_deepstoreUploadRetryTimeoutMs`. That would require adding another method to the FileUploadDownloadClient. Since that class is already quite large, I was hesitant on adding another method there (since it doesn't seem super necessary to have).
--
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 #10752: [Draft] Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10752:
URL: https://github.com/apache/pinot/pull/10752#issuecomment-1544907790
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10752?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 [#10752](https://app.codecov.io/gh/apache/pinot/pull/10752?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2929f9a) into [master](https://app.codecov.io/gh/apache/pinot/commit/fe98bb0783213504c77be397b1750de3962000ef?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fe98bb0) will **decrease** coverage by `46.33%`.
> The diff coverage is `55.17%`.
> :exclamation: Current head 2929f9a differs from pull request most recent head 29cbc8d. Consider uploading reports for the commit 29cbc8d to get more accurate results
```diff
@@ Coverage Diff @@
## master #10752 +/- ##
=============================================
- Coverage 70.45% 24.12% -46.33%
+ Complexity 5646 49 -5597
=============================================
Files 2153 2137 -16
Lines 115396 115003 -393
Branches 17368 17318 -50
=============================================
- Hits 81305 27749 -53556
- Misses 28449 84326 +55877
+ Partials 5642 2928 -2714
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.12% <55.17%> (-0.12%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `?` | |
| 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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...apache/pinot/common/utils/SimpleHttpErrorInfo.java](https://app.codecov.io/gh/apache/pinot/pull/10752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvU2ltcGxlSHR0cEVycm9ySW5mby5qYXZh) | `60.00% <ø> (ø)` | |
| [.../data/manager/realtime/PinotFSSegmentUploader.java](https://app.codecov.io/gh/apache/pinot/pull/10752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUGlub3RGU1NlZ21lbnRVcGxvYWRlci5qYXZh) | `15.55% <0.00%> (-68.54%)` | :arrow_down: |
| [...che/pinot/server/api/resources/TablesResource.java](https://app.codecov.io/gh/apache/pinot/pull/10752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvYXBpL3Jlc291cmNlcy9UYWJsZXNSZXNvdXJjZS5qYXZh) | `24.30% <0.00%> (-11.45%)` | :arrow_down: |
| [.../core/realtime/PinotLLCRealtimeSegmentManager.java](https://app.codecov.io/gh/apache/pinot/pull/10752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1Bpbm90TExDUmVhbHRpbWVTZWdtZW50TWFuYWdlci5qYXZh) | `60.77% <63.15%> (-14.72%)` | :arrow_down: |
| [...ger/realtime/Server2ControllerSegmentUploader.java](https://app.codecov.io/gh/apache/pinot/pull/10752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VydmVyMkNvbnRyb2xsZXJTZWdtZW50VXBsb2FkZXIuamF2YQ==) | `67.56% <75.00%> (-9.58%)` | :arrow_down: |
| [...va/org/apache/pinot/controller/ControllerConf.java](https://app.codecov.io/gh/apache/pinot/pull/10752?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `53.00% <100.00%> (-6.86%)` | :arrow_down: |
... and [1620 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10752/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] chenboat commented on a diff in pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1195833919
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java:
##########
@@ -468,7 +468,8 @@ public Response downloadValidDocIds(
* Upload a low level consumer segment to segment store and return the segment download url. This endpoint is used
* when segment store copy is unavailable for committed low level consumer segments.
* Please note that invocation of this endpoint may cause query performance to suffer, since we tar up the segment
- * to upload it.
+ * to upload it. There's a query parameter to configure how long the segment-uploader should wait for the upload to
+ * deep-store to finish.
Review Comment:
Can you add javadoc on what default value -1 means?
--
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] ankitsultana commented on a diff in pull request #10752: [Draft] Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1191769830
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/SimpleHttpErrorInfo.java:
##########
@@ -30,6 +31,7 @@ public class SimpleHttpErrorInfo {
private String _error;
@JsonCreator
+ @JsonIgnoreProperties(ignoreUnknown = true)
Review Comment:
I added this because I saw an issue in our prod cluster where the controller was not logging the exact error message returned by the server because an unexpected field was returned in the response (I think it was `status`).
--
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] chenboat commented on a diff in pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1195835315
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentUploader.java:
##########
@@ -25,5 +25,15 @@
public interface SegmentUploader {
// Returns the URI of the uploaded segment. null if the upload fails.
+
+ /**
+ * Uploads the given segmentFile to the deep-store. The upload to deep-store involve calling another Pinot component.
+ */
Review Comment:
Can you combine the javadoc in line 27 in the new comment too? What does "involve calling another Pinot component" mean? Can you elaborate? It is not clear.
--
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 a diff in pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1196924108
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1555,4 +1562,19 @@ public PauseStatus getPauseStatus(String tableNameWithType) {
Set<String> consumingSegments = findConsumingSegments(idealState);
return new PauseStatus(Boolean.parseBoolean(isTablePausedStr), consumingSegments, null);
}
+
+ @VisibleForTesting
+ String moveSegmentFile(String rawTableName, String segmentName, String segmentLocation, PinotFS pinotFS)
+ throws IOException {
+ URI segmentFileURI = URIUtils.getUri(segmentLocation);
+ URI uriToMoveTo = createSegmentPath(rawTableName, segmentName);
Review Comment:
+1 to handle this inside move()
--
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] ankitsultana commented on a diff in pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1196894903
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1555,4 +1562,19 @@ public PauseStatus getPauseStatus(String tableNameWithType) {
Set<String> consumingSegments = findConsumingSegments(idealState);
return new PauseStatus(Boolean.parseBoolean(isTablePausedStr), consumingSegments, null);
}
+
+ @VisibleForTesting
+ String moveSegmentFile(String rawTableName, String segmentName, String segmentLocation, PinotFS pinotFS)
+ throws IOException {
+ URI segmentFileURI = URIUtils.getUri(segmentLocation);
+ URI uriToMoveTo = createSegmentPath(rawTableName, segmentName);
Review Comment:
Good point.
Ideally PinotFS should be able to handle this since `PinotFS::move(source=foo, destination=foo)` is a valid call.
--
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] chenboat commented on a diff in pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1195840785
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1404,6 +1405,8 @@ public void uploadToDeepStoreIfMissing(TableConfig tableConfig, List<SegmentZKMe
retentionMs - MIN_TIME_BEFORE_SEGMENT_EXPIRATION_FOR_FIXING_DEEP_STORE_COPY_MILLIS);
}
+ PinotFS pinotFS = createPinotFS(rawTableName);
Review Comment:
PinotFS is not per table but per Pinot cluster. It is a bit strange to create the PinotFS from a table name. After looking through the two private methods added, it is much more clearer if we can get the PinotFS based on the scheme part of _controllerConf.getDataDir().
--
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] chenboat commented on a diff in pull request #10752: Minor Pinot Deepstore Upload Retry Task Improvements
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on code in PR #10752:
URL: https://github.com/apache/pinot/pull/10752#discussion_r1195834151
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java:
##########
@@ -527,7 +529,12 @@ public String uploadLLCSegment(
// Use segment uploader to upload the segment tar file to segment store and return the segment download url.
SegmentUploader segmentUploader = _serverInstance.getInstanceDataManager().getSegmentUploader();
- URI segmentDownloadUrl = segmentUploader.uploadSegment(segmentTarFile, new LLCSegmentName(segmentName));
+ URI segmentDownloadUrl;
+ if (timeoutMs <= 0) {
Review Comment:
javadoc to explain the difference here?
--
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