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

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

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


   > 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.
   
   I don't have a strong opinion on this. The logic looks good to me. But the more descriptive name provides benefits:
   1. Doesn't confuse code reader, reader won't confuse a plain text file with an encrypted file.
   2. Better naming usually means better abstraction, and more future-proof. However, sometimes, it is kind of over-engineered, but less likely, and need to be analyzed case by case.


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