Hive encryption nits #14659
Hive encryption nits #14659
Conversation
| if (keyManagementClient == null) { | ||
| throw new RuntimeException( | ||
| "Cant create encryption manager, because key management client is not set"); | ||
| "Cannot create encryption manager without a key management client"); |
There was a problem hiding this comment.
This is a nit, matching the "Cannot" language used elsewhere. (Not strongly opinionated though)
There was a problem hiding this comment.
We could take this further with maybe:
throw new RuntimeException(
"Cannot create encryption manager without a key management client. Consider setting the '"
+ CatalogProperties.ENCRYPTION_KMS_IMPL + "' catalog property.");However, this might be a bit premature
There was a problem hiding this comment.
We could also throw a different exception class here
There was a problem hiding this comment.
IllegalArgs or a Precondition prompting the key to be set sounds reasonable to me
There was a problem hiding this comment.
+1 to use IllegalArgumentException or Preconditions check.
There was a problem hiding this comment.
Thanks all, have made the change. You now get:
java.lang.IllegalArgumentException: Cannot create encryption manager without a key management client. Consider setting the 'encryption.kms-impl' catalog property
at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:217)
| encryptionProperties.put( | ||
| TableProperties.ENCRYPTION_DEK_LENGTH, String.valueOf(encryptionDekLength)); | ||
| Map<String, String> encryptionProperties = | ||
| ImmutableMap.of( |
There was a problem hiding this comment.
In #13225, I felt like ImmutableMap.of would be more natural here
|
|
||
| if (removedProps.contains(TableProperties.ENCRYPTION_TABLE_KEY)) { | ||
| throw new RuntimeException("Cannot remove key in encrypted table"); | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
This follows the discussion #13225 (comment). To me, IllegalArgumentException makes sense
| assertThatThrownBy( | ||
| () -> sql("ALTER TABLE %s UNSET TBLPROPERTIES (`encryption.key-id`)", tableName)) | ||
| .hasMessageContaining("Cannot remove key in encrypted table"); | ||
| .hasMessageContaining("Cannot remove encryption key ID from an encrypted table"); |
There was a problem hiding this comment.
can we check the execption class too ?
| if (keyManagementClient == null) { | ||
| throw new RuntimeException( | ||
| "Cant create encryption manager, because key management client is not set"); | ||
| "Cannot create encryption manager without a key management client"); |
There was a problem hiding this comment.
IllegalArgs or a Precondition prompting the key to be set sounds reasonable to me
3 participants
Minor clean-ups to Hive encryption integration that arose from #13225
cc @ggershinsky @huaxingao