[GitHub] [druid] pjain1 commented on pull request #11507: option to use deep storage for storing shuffle data

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

pjain1 commented on pull request #11507:
URL: https://github.com/apache/druid/pull/11507#issuecomment-891161308


   > Seems like only subTaskId and loadSpec are in use in this class which confuses me why this class needs other fields. I suggest to add a new interface for PartitionLocation and let DeepStoragePartitionLocation implement it instead of extending GenericPartitionLocation because GenericPartitionLocation is designed for local storage for shuffle. Since we already have an abstract class of the same name of PartitionLocation, you will have to rename it or use another name for the new interface.
   
   @jihoonson I moved PartitionLocation to be an interface but realized there are other abstract classes like `PartialSegmentMergeTask`, `PartialSegmentMergeIOConfig`, `PartialSegmentMergeIngestionSpec` similar to `PartitionLocation` which will ideally also need to be made interfaces and each having two implementations based on `PartitionLocation` class. However I don't see what will be gained from interfacing these as the logic for these classes will be same for any PartitionLocation as of now, seems like generalizing too much. I think we can keep `DeepStoragePartitionLocation` an extension of `GenericPartitionLocation` to avoid this problem but some of the fields would be redundant. What do you think ?


-- 
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@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org