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/06/27 19:19:36 UTC
[GitHub] [incubator-pinot] elonazoulay opened a new pull request #5632: Fix realtime segment download url
elonazoulay opened a new pull request #5632:
URL: https://github.com/apache/incubator-pinot/pull/5632
If data directory is not using local fs then return
the url using the configured data directory.
## Description
The realtime segment url was always using the controller vip, it should use the configured data directory.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.
If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text
## 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 #5632: Fix realtime segment download url
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5632:
URL: https://github.com/apache/incubator-pinot/pull/5632#issuecomment-655676032
closing since this is already handled in another PR.
----------------------------------------------------------------
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] chenboat commented on pull request #5632: Fix realtime segment download url
Posted by GitBox <gi...@apache.org>.
chenboat commented on pull request #5632:
URL: https://github.com/apache/incubator-pinot/pull/5632#issuecomment-654626458
> now that #5632 is merged, can we close this PR
yes. @elonazoulay please let me know if you have any issue with #5632.
----------------------------------------------------------------
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 #5632: Fix realtime segment download url
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5632:
URL: https://github.com/apache/incubator-pinot/pull/5632#issuecomment-651585940
@chenboat, is there a PR associated here. I thought the PR you were working on was making deep store optional for some cases.
@mcvsubbu, there are two solutions
1. Let the real-time servers push the segments to controllers but the controller puts the deep store uri in zk segment metadata
2. real-time server commits segment directly to deep store and invoke the commit API with URI?
I thought this PR was achieving 1 and we can get to 2 in the next iteration.
----------------------------------------------------------------
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] chenboat commented on a change in pull request #5632: Fix realtime segment download url
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5632:
URL: https://github.com/apache/incubator-pinot/pull/5632#discussion_r448030685
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java
##########
@@ -485,9 +487,7 @@ private LLCRealtimeSegmentZKMetadata updateCommittingSegmentZKMetadata(String re
// TODO Issue 5953 remove the long parsing once metadata is set correctly.
committingSegmentZKMetadata.setEndOffset(committingSegmentDescriptor.getNextOffset());
committingSegmentZKMetadata.setStatus(Status.DONE);
- committingSegmentZKMetadata.setDownloadUrl(URIUtils
- .constructDownloadUrl(_controllerConf.generateVipUrl(), TableNameBuilder.extractRawTableName(realtimeTableName),
- segmentName));
+ committingSegmentZKMetadata.setDownloadUrl(getDownloadUrl(TableNameBuilder.extractRawTableName(realtimeTableName), segmentName));
Review comment:
If I understand correctly, the goal of this PR is to provide a download url which is not a hardcoded controller vip based. That is a good idea. IMO, the cleaner way is to set the download url through the input _committingSegmentDescriptor's_ _segmentLocation_ field. The _segmentLocation_ field can be set in the caller side as shown below. Note we just need change _commitSegmentFile_() to return the final uri string instead of void.
https://github.com/apache/incubator-pinot/blob/5e532eccd8f0898b0b13eef32b9f5335682a037e/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/SegmentCompletionManager.java#L1073-L1081
The main benefit is that we do not need the downloadUrl() method below anymore because it has already been done below: -- so no code duplication at least for the non local scheme.
https://github.com/apache/incubator-pinot/blob/5e532eccd8f0898b0b13eef32b9f5335682a037e/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java#L371
----------------------------------------------------------------
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 #5632: Fix realtime segment download url
Posted by GitBox <gi...@apache.org>.
kishoreg commented on pull request #5632:
URL: https://github.com/apache/incubator-pinot/pull/5632#issuecomment-654578042
now that https://github.com/apache/incubator-pinot/pull/5632 is merged, can we close this PR
----------------------------------------------------------------
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] chenboat edited a comment on pull request #5632: Fix realtime segment download url
Posted by GitBox <gi...@apache.org>.
chenboat edited a comment on pull request #5632:
URL: https://github.com/apache/incubator-pinot/pull/5632#issuecomment-652108614
> @chenboat can you review this PR and let us know if we can go ahead with this?
@kishoreg I left my review on this PR. This PR has a big overlap with my standalone PR #5639 which is ready for review now. My suggestion (which could be biased) is to focus on PR #5639 which also includes a fix for the same problem. Otherwise we are wasting the review efforts. I would invite both @elonazoulay and @mcvsubbu to review that PR.
----------------------------------------------------------------
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 closed pull request #5632: Fix realtime segment download url
Posted by GitBox <gi...@apache.org>.
kishoreg closed pull request #5632:
URL: https://github.com/apache/incubator-pinot/pull/5632
----------------------------------------------------------------
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