Thanks Xiao Chen for working on this.
Seems like a good change.
I have some review comments from the client side.
Will review server side changes later.
1. If I understand the patch correctly, this server request will be either All or None.
I haven't seen
HDFS-10899, so I might miss some context.
Either all the EDEKs within the list will be re-encrypted or if anyone of ekv encounters Exception, then the whole call will fail.
It would be better if we don't fail if one EDEK fails to process.
We can return Map<originalEKV, re-encryptedEKV>. In case of failed operations, we can just set null and traverse the map on the client side to see which ekv's failed.
Instead of creating CryptoCodec and Encryptor for every ekv, we can create it just once.
I don't think it stores some state related to each ekv.
Also we need to close CryptoCodec instance.
I would use try with resources block.
In reencryptEncryptedKeys method, we need to add null check also along with zero length array.
if (ekvs == null || ekvs.isEmpty())
toJSON(EncryptedKeyVersion encryptedKeyVersion) and toJSON(KeyProvider.KeyVersion keyVersion)
Why do we need to use LinkedHashMap ?
I don't think in the server side, while iterating over the map, we care about the insertion order.
toJSON(EncryptedKeyVersion encryptedKeyVersion, String keyName)
Will the keyName will ever be null ?
We do a "not null" check in the calling method KMSClientProvider#reencryptEncryptedKeys(List<EncryptedKeyVersion> ekvs)
checkNotNull and checkNotEmpty
Both of these methods are actually utility methods. I would rather see this in KMSUtil instead of KMSClientProvider