Repository navigation
GH-40731: [C++][Parquet] Minor enhancement code of encryption - #40732
Conversation
|
|
| std::shared_ptr<FileKeyMaterialStore> key_material_store) | ||
| : FileKeyUnwrapper(/*key_toolkit_owner=*/nullptr, key_toolkit, kms_connection_config, | ||
| cache_lifetime_seconds, /*file_path=*/"", | ||
| cache_lifetime_seconds, std::move(key_material_store), |
There was a problem hiding this comment.
@adamreeve I found previously this key_material_store is not used. Is this expected?
There was a problem hiding this comment.
It seems that FileKeyMaterialStore is only required when master key rotation is used. Otherwise it is null and created internally based on file path and file system.
There was a problem hiding this comment.
It would be good for @adamreeve to double check if this is required. If not, we probably should remove the parameter to reduce confusion.
There was a problem hiding this comment.
This is unexpected. It looks like during the process of working on #34181 I refactored things so that this parameter was no longer used but didn't realise this. The FileKeyUnwrapper created in KeyToolkit::RotateMasterKeys is only ever used to call GetDataEncryptionKey, which takes a KeyMaterial parameter so bypasses the key_material_store_ completely, and there's no reason it should need to be passed in to the constructor.
I also added an incorrect comment about this constructor overload in file_key_unwrapper.h, saying, "This is useful for key rotation to allow accessing the key material store after it is used." I guess I got myself confused with the use of the temp_key_material_store passed to the FileKeyWrapper.
It seems to me that this overload can be removed completely, and KeyToolkit::RotateMasterKeys can use the other constructor. The file_path and filesystem could be provided but won't be used.
There was a problem hiding this comment.
Oh actually it looks like the constructor parameter was being used prior to #40329, I didn't notice an extra commit was added to that PR to refactor the constructor parameters making it no longer used (7547c79). But even though it was being used to set key_material_store_ before, that was never actually required for KeyToolkit::RotateMasterKeys and the extra constructor overload wasn't needed.
There was a problem hiding this comment.
So my edit is correct? Or I should removing this? I'm not sure would legacy user using this
There was a problem hiding this comment.
Yeah I think it depends on whether removing the overload is likely to affect any users. Your change is correct, but @pitrou's change not breaking anything exposed that the extra overload wasn't really needed by KeyToolkit::RotateMasterKeys. I guess it doesn't hurt to keep it as is, but it might be nice to remove the overload to simplify things.
There was a problem hiding this comment.
I agree with the current change to fix the missing argument instead of removing a public function.
There was a problem hiding this comment.
Can you open an issue for exploring whether the overload can be removed? In the meantime we can merge this PR.
|
Also cc @pitrou |
|
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 145c4d3. There was 1 benchmark result with an error:
There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 26 possible false positives for unstable benchmarks that are known to sometimes produce them. |
#40808) ### Rationale for this change I added this comment in #34181, but from the discussion in #40732 (comment), I realised this comment was incorrect. The extra overload appears to just be a convenience as a `FileKeyMaterialStore` is already constructed in `KeyToolkit::RotateMasterKeys`, but the store isn't actually used by the `FileKeyUnwrapper` in that method, as only `FileKeyUnwrapper::GetDataEncryptionKey` is called, which bypasses the store. `RotateMasterKeys` does however rely on the `temp_key_material_store` passed to the `FileKeyWrapper` being used, which is possibly where this confusion came from. ### What changes are included in this PR? Removes an incorrect statement from a C++ header comment. ### Are these changes tested? NA ### Are there any user-facing changes? No Authored-by: Adam Reeve <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
apache#40808) ### Rationale for this change I added this comment in apache#34181, but from the discussion in apache#40732 (comment), I realised this comment was incorrect. The extra overload appears to just be a convenience as a `FileKeyMaterialStore` is already constructed in `KeyToolkit::RotateMasterKeys`, but the store isn't actually used by the `FileKeyUnwrapper` in that method, as only `FileKeyUnwrapper::GetDataEncryptionKey` is called, which bypasses the store. `RotateMasterKeys` does however rely on the `temp_key_material_store` passed to the `FileKeyWrapper` being used, which is possibly where this confusion came from. ### What changes are included in this PR? Removes an incorrect statement from a C++ header comment. ### Are these changes tested? NA ### Are there any user-facing changes? No Authored-by: Adam Reeve <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
Rationale for this change
std::movein encryptionNULLPTRrather thanNULL.What changes are included in this PR?
Applying more
std::movein encryptionAre these changes tested?
No need
Are there any user-facing changes?
No