Basic manifest encryption #8252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ggershinsky
force-pushed
the
manifest-encryption
branch
from
754a597
to
e0c55d9
Compare
ggershinsky
force-pushed
the
manifest-encryption
branch
from
73892a1
to
afa086b
Compare
update unitest cleanup update comments update manifest encryption fix benchmark compilation undo spark 3.4 changes wrong constructor add constructor for old spark tests leverage EncryptingFileIO
ggershinsky
force-pushed
the
manifest-encryption
branch
from
afa086b
to
d0d9d28
Compare
ggershinsky
commented
Feb 28, 2024
| @@ -290,7 +295,7 @@ static class V1Writer extends ManifestWriter<DataFile> { | |||
| private final V1Metadata.IndexedManifestEntry entryWrapper; | |||
|
|
|||
| V1Writer(PartitionSpec spec, OutputFile file, Long snapshotId) { | |||
| super(spec, file, snapshotId); | |||
| super(spec, EncryptedFiles.plainAsEncryptedOutput(file), snapshotId); | |||
There was a problem hiding this comment.
todo: accept EncryptedOutputFile, and throw exception if key metadata is not null / empty.
ggershinsky
commented
Feb 28, 2024
core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java
Outdated
Show resolved
Hide resolved
ggershinsky
commented
Feb 28, 2024
core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java
Show resolved
Hide resolved
rdblue
reviewed
Feb 28, 2024
api/src/main/java/org/apache/iceberg/io/PositionOutputStream.java
Outdated
Show resolved
Hide resolved
|
@rdblue @RussellSpitzer thanks for the comments. The last commit addresses them. |
| @@ -499,21 +500,34 @@ protected OutputFile manifestListPath() { | |||
| "snap-%d-%d-%s", snapshotId(), attempt.incrementAndGet(), commitUUID)))); | |||
| } | |||
|
|
|||
| /** | |||
| * @deprecated since 1.6.0 . Will be removed in 1.7.0. Use {@link | |||
There was a problem hiding this comment.
I think we usually add in the deprecation notes right before release (I don't think there will be a 1.6 or 1.7
There was a problem hiding this comment.
I think we do want to add these right away. If we don't have a 1.7 then we'll rewrite to "removed in 2.0"
rdblue
reviewed
Mar 11, 2024
rdblue
reviewed
Mar 11, 2024
rdblue
reviewed
Mar 11, 2024
rdblue
reviewed
Mar 11, 2024
rdblue
reviewed
Mar 11, 2024
core/src/test/java/org/apache/iceberg/TestManifestEncryption.java
Outdated
Show resolved
Hide resolved
rdblue
reviewed
Mar 11, 2024
rdblue
reviewed
Mar 11, 2024
rdblue
reviewed
Mar 11, 2024
core/src/test/java/org/apache/iceberg/TestManifestEncryption.java
Outdated
Show resolved
Hide resolved
rdblue
approved these changes
Mar 11, 2024
Co-authored-by: Ryan Blue <blue@apache.org>
rdblue
reviewed
Mar 13, 2024
|
|
||
| private static final DataFile DATA_FILE = | ||
| new GenericDataFile( | ||
| 0, |
There was a problem hiding this comment.
Nit: this should also be SPEC.specId()
There was a problem hiding this comment.
Sounds good, I'll fix this in the next patch.
rdblue
approved these changes
Mar 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The basics, and support for Spark core. No support yet for Flink or for Spark actions, benchmarks or procedures.