amogh-jahagirdar commented on code in PR #15911:
URL: https://github.com/apache/iceberg/pull/15911#discussion_r3253264855
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -56,18 +61,30 @@ public class BaseDVFileWriter implements DVFileWriter {
private final Function<String, PositionDeleteIndex> loadPreviousDeletes;
private final Map<String, Deletes> deletesByPath = Maps.newHashMap();
private final Map<String, BlobMetadata> blobsByPath = Maps.newHashMap();
+ private final EncryptionKeyMetadata keyMetadata;
private DeleteWriteResult result = null;
public BaseDVFileWriter(
OutputFileFactory fileFactory, Function<String, PositionDeleteIndex>
loadPreviousDeletes) {
- this(() -> fileFactory.newOutputFile().encryptingOutputFile(),
loadPreviousDeletes);
+ this(fileFactory.newOutputFile(), loadPreviousDeletes);
}
public BaseDVFileWriter(
- Supplier<OutputFile> dvOutputFile,
+ EncryptedOutputFile encryptedOutputFile,
Review Comment:
Instead of changing the constructor to force callers to pass in an
`EncryptedOutputFile`, could we keep this as is and then when we produce the
OutputFile on flush, if it's an instanceOf EncryptedOutputFile, we can extract
the key metadata that way when producing the DV metadata. I think that makes
the change smaller and keeps the writer abstraction from not pushing the
encryption concept to callers and keeping that internal to the implementation.
The abstraction is left generic enough so that if a caller cares about
encryption, it's on them to create the right the output file or factory.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
