Core: ManifestListWriter cleanup around encryption keys - #18150
Conversation
Problems this solves: - There is a dependency between toManifestFile() and encryptionKeys(). - From toManifestFile()'s return type only keyId is used, but it's also present in the output of encryptionKeys() - manifestListFile member can be dropped - outputFile member can be dropped
| private final FileAppender<ManifestFile> writer; | ||
| private final StandardEncryptionManager standardEncryptionManager; | ||
| private final NativeEncryptionKeyMetadata manifestListKeyMetadata; | ||
| private final OutputFile outputFile; |
There was a problem hiding this comment.
The only usage of the outputFile member outside of the constructor was to create a ManifestListFile in toManifestListFile(), but then it was unused later on. Now that the functions are merged, no need for this member
There was a problem hiding this comment.
Agreed, even if we do my proposed approach we can drop ManifestListFile. Though in my approach, we would need OutputFile, which I feel like is very reasonable and consistent with what we do for "regular" manifests
. The writer needs to have some reference to an output file
| List<ManifestFile> manifests = | ||
| ManifestLists.read( | ||
| readingIO.newInputFile( | ||
| new BaseManifestListFile( |
There was a problem hiding this comment.
Note, #17545 will get rid of BaseManifestListFile and use a more general version of newInputFile
|
Hi @amogh-jahagirdar @smaheshwar-pltr @nssalian , I think #17984 was merged in a premature state and we could have made a final cleanup on the code on the area of |
| return null; | ||
| } | ||
|
|
||
| public ManifestListFile toManifestListFile() { |
There was a problem hiding this comment.
I agree that the dependency is weird, it's what I mentioned here already; we should clean that part up.
But I think there's a good argument for having a toManifestListFile API, it's consistent with what we have for manifest files (toManifestFile) and I think fundamentally a writer handing back a "This is the complete object" makes it easier for consumers to just use that as a source of truth (e.g. they can do manifestList.encryptionKeys(), manifestList.location() etc).
In our case the consumer is just SnapshotProducer
So what I was thinking was a good way to clean this up would be
EncryptionKeys encryptionKeys() {
/// what you have now
}
ManifestListFile toManifestList() {
EncryptionKeys encryptionKeys = encryptionKeys()
return new ManifestListFile(location, encryptionKeys != null ? encryptionKeys.fileKey().keyId() : null):
}
So we get rid of the bad dependency ordering and still keep (imo) a useful API.
WDYT?
There was a problem hiding this comment.
I'm fine as it is btw, we can always add functions later, but just wanted to point out that the toManifestListFile was something thought through and not rushed :) but agree on cleaning up the dependency/state, that's a good thing.
There was a problem hiding this comment.
Thanks for the feedback, @amogh-jahagirdar !
As you pointed out SnapshotProducer is the only user of toManifestList(), but in practice the ManifestListFile return type is not needed there, just the key ID from it. In fact, I'm working on dropping this whole ManifestListFile abstraction because there is nothing specific to manifest files and we need a way to handle manifest files, root manifest, snapshot files in the same way in term of encryption.
| } | ||
|
|
||
| ManifestListFile manifestListFile = writer.toManifestListFile(); | ||
| this.manifestListEncryptionKeys = writer.encryptionKeys(); |
There was a problem hiding this comment.
If we do what I mentioned above, I think we can effectively get rid of this state on SnapshotProducer, and just pass in manifestListFile.encryptionKeys(). So it trades off more state in ManifestListFile (OutputFile) for getting rid of state in SnapshotProducer, which I think is better as as it really shouldn't be SnapshotProducer's responsibility imo.
| } | ||
|
|
||
| ManifestListFile manifestListFile = writer.toManifestListFile(); | ||
| assertThat(writer.toManifestListFile().encryptionKeyID()) |
There was a problem hiding this comment.
One possible issue with removing this is that then we're not effectively testing the encryptionKeys == null which currently prevents registering 2 different keys for the same file. But fwiw the utility of testing this behavior is moot if we're only calling this from snapshot producer just once. So I'm good with the removal.
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @gaborkaszab , While I still feel like a toManifestListFile is useful we can table that discussion point, the PR is a clear improvement over the current state (removing manifest list file state and not requiring toManifestListFile() in order to setup encryptionKeys) which is appreciated, so I'll go ahead and merge.
2 participants
Problems this solves: