[GitHub] [iceberg] jackye1995 commented on pull request #2520: Core: add key_metadata in ManifestFile

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

jackye1995 commented on pull request #2520:
URL: https://github.com/apache/iceberg/pull/2520#issuecomment-841430136


   Sorry I was a bit distracted and did not update this PR for a while.
   
   @rdblue thanks for the suggestion for refactoring, I actually thought about the approach you considered, and my major concern is that we are creating 2 different code paths for using or not using encryption. On the other hand, for data file encryption, Iceberg is currently having a single unified code path for always using encryption, with the default encryption manager to be the plaintext manager. I think it would be good to keep things consistent. 
   
   I completely agree that the use of a global default encryption manager class is not necessary, and I will remove that. What I hope to achieve is that the method `ManifestFiles.read(ManifestFile manifest, FileIO io, Map<Integer, PartitionSpec> specsById)` is completely not used and deprecated, and all the callers should use the method with encryption, and supply `table.encryption()` as the input to the encryption manager. I was trying to avoid too many single line changes in this PR, but my final intention became not clear, so I will update based on that and we can reevaluate the situation.
   
   For what @flyrain and @RussellSpitzer concern, I think I can make it better by directly passing in `EncryptedOutputFIle.encryptingOutputFile()` to minimize interface change, will notify you guys once I update.


-- 
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: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org