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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
