[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5639: Refactor how Pinot controller stores segment download url in Zookeeper to deal with peer uri format

[GitBox <gi...@apache.org>]

mcvsubbu commented on a change in pull request #5639:
URL: https://github.com/apache/incubator-pinot/pull/5639#discussion_r448536743



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/SegmentCompletionManager.java
##########
@@ -1068,16 +1069,28 @@ private int numReplicasToLookFor() {
       _state = State.COMMITTING;
       // In case of splitCommit, the segment is uploaded to a unique file name indicated by segmentLocation,
       // so we need to move the segment file to its permanent location first before committing the metadata.
-      if (isSplitCommit) {
+      // The committingSegmentDescriptor is then updated with the permanent segment location to be saved in metadata
+      // store.
+      // The exception is when the server uses the Peer segment download scheme, in such case, there is no need to
+      // move the segment.
+      if (isSplitCommit && !isPeerSegmentDownloadScheme(committingSegmentDescriptor)) {

Review comment:
       we dont need to check the peer download scheme here, since we check and put the right value while committing metadata in PinotLLCRealtimeSegmentManager. We can always set the segment location. The if condition can be as before

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java
##########
@@ -355,8 +355,9 @@ void setIdealState(String realtimeTableName, IdealState idealState) {
    * This method moves the segment file from another location to its permanent location.
    * When splitCommit is enabled, segment file is uploaded to the segmentLocation in the committingSegmentDescriptor,
    * and we need to move the segment file to its permanent location before committing the segment metadata.
+   * Return the permanent location of the segment if the commit succeeds.
    */
-  public void commitSegmentFile(String realtimeTableName, CommittingSegmentDescriptor committingSegmentDescriptor)
+  public String commitSegmentFile(String realtimeTableName, CommittingSegmentDescriptor committingSegmentDescriptor)

Review comment:
       check the descriptor for peer  scheme in this method, and move only if it is not peer scheme. leave the descriptor.location as is if it is peer scheme. If not, update the location with the final location in this method. You can still return void.




----------------------------------------------------------------
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