[GitHub] [incubator-pinot] kishoreg commented on pull request #5712: Add startBatchUpload, endBatchUpload controller API

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

kishoreg commented on pull request #5712:
URL: https://github.com/apache/incubator-pinot/pull/5712#issuecomment-662164476


   > > > > I think it is prudent to add an extra query parameter to the controller API for start/end batchupload. I suggest "operation" and make it string type. In this case, "Operation" will be set to "MERGE"
   > > > > This way, we don't assume that if source segments is null, the the operation MUST be new segment upload. Let us specify the operation clearly, and then it gets easier to evolve the API over other operations we may need.
   > > > 
   > > > 
   > > > @mcvsubbu We want to build primitive operations and we can use them to achieve multiple purpose such as batch upload, batch replace and version control. I don't think we should associate this primitive operation with any specific operation such as `MERGE` or `NEW_UPLOAD`. The semantic of this primitive operation is quite clear: replace the segments in `segmentsFrom` with segments in `segmentsTo` (we might want to rename the parameters if that makes the semantic more clear). @snleee Thoughts?
   > > 
   > > 
   > > If we know that this API will never be used for anything other than merge, then it is ok as defined. In that case, I would rename the API to say startBatchForMerge() or something like that. That way, if we want to introduce a startBatch for upload, we can do so.
   > > But then we know that this API will perhaps be re-used for batch upload of segments. Since we are reasonably sure that will be the case, but we don't know exactly what arguments it will take when we re-use it for another operation, it is best to specify the operation name we want.
   > > that way, even if some other set of arguments need to be added, or some null assimptions do not hold true, we have the operation very clearly specified. Yes, we can always add operaton later on, and say that if "operation" is not present, then treat it as merge, but I think it is clearner to specify that now.
   > 
   > This API will be used for merge, batch upload, batch replace, but I don't want to associate the API with certain type of operation because the semantic of this API is very clear and is independent of the actual operation. It will replace the segments in `segmentsFrom` with segments in `segmentsTo` atomically, and this will hold for all operations.
   
   If thats the case, then I agree with Subbu. It's better to name this primitive as ReplaceSegments or SwapSegments. Then all higher-level API can leverage this. WDYT?


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