adamreeve commented on issue #46217: URL: https://github.com/apache/arrow/issues/46217#issuecomment-2825621423
`KeyToolkit` is technically public so this would be a breaking change, but I don't think this was intentional. Eg. for key rotation users should be using the `CryptoFactory`, so I think it's OK to change the `KeyToolkit` API in theory. > except that the timestamp is set to now + cache_cleanup_period_seconds, which looks like a bug. Yes that definitely looks like a bug and should be fixed. I don't think you can simply remove the cleanup logic from `RotateMasterKeys` though. It's important that that has its own "last clean up" time, as calling `RotateMasterKeys` is an indication that the master keys have been updated so key encryption keys need to be re-encrypted, and the full KEK cache should immediately be cleared including keys that have not yet expired. The reason this isn't done on every call to `RotateMasterKeys` is that after master keys have been updated you'll often want to re-encrypt DEKs for multiple files and re-use the new KEKs. There are also other methods on `KeyToolkit` that require access to multiple members, eg. `GetKmsClient`, `RemoveCacheEntriesForToken` and `RemoveCacheEntriesForAllTokens`. So the current structure of `KeyToolkit` being a class with these methods makes sense to me, and I don't see much benefit in changing it to a struct and separate free functions. -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
