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

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

jackye1995 edited a comment on pull request #2520:
URL: https://github.com/apache/iceberg/pull/2520#issuecomment-841538934


   @rdblue @flyrain @RussellSpitzer I have updated this PR with all the changes I expected to make in several PRs, it is a lot of files but I think it makes the discussion much easier to show them in a single place.
   
   The key thing we need to determine is: should we separate the code path for manifest read/write with and without encryption. My stand is that we should not separate it, because:
   1. data file read/write does not separate it
   2. managing things in a single  code path is much easier in long run
   
   With this assumption, the key principles I followed for the changes are:
   1. for every method in `ManifestFiles`, create a new version that takes encryption manager, and deprecate the old methods
   2. change the callers of old methods all the way to the top, and deprecate any related public methods.
   3. use `EncryptionManagers.plaintext()` for all those deprecated methods so that we don't create a lot of redundant plain text manager.
   
   Regarding the concern brought up by @RussellSpitzer and @flyrain , my current take is that we have to pass `EncryptedOutputFile` all the way down to the place where the `encryptedOutputFile.keyMetadata()` is written by the engine to the actual manifest list. This does make the naming a bit awkward as @flyrain suggested, but I think this is the same strategy taken by the data file write path, where we can notice that the `FileAppenderFactory` also have those methods containing `EncryptedOutputFile` for exactly the same purpose.
   
   Regarding `EncryptionManagers` that @rdblue talked about removing, as I said in point 3 before, now I am treating it as the static factory to get a plain text manager singleton and that is only used for deprecated methods. I do expect to have a second usage of it, similar to `LocationProviders`, to add a method `EncryptionManagers.load(...)` to load a custom encryption manager and allow a `Catalog` to initialize a custom encryption manager for a `TableOperations`.


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