Encryption: Simplify Hive key handling and add transaction tests#14752
Encryption: Simplify Hive key handling and add transaction tests#14752huaxingao merged 2 commits intoapache:mainfrom
Conversation
| : TableProperties.ENCRYPTION_DEK_LENGTH_DEFAULT; | ||
|
|
||
| encryptedKeysFromMetadata = Optional.ofNullable(current().encryptionKeys()); | ||
| encryptedKeys = |
There was a problem hiding this comment.
If we can assume that encryption properties are unchanging (aside from keys) and therefore do not need to be updated on refresh, there's a actually simpler approach that just adds metadata keys to the current EM's transient state: https://github.com/apache/iceberg/compare/main...smaheshwar-pltr:iceberg:sm/add-keys?expand=1.
We could also go with that approach, and introduce setters in case we can't make that assumption. But then I suspect just re-initialising the EM on refresh makes more sense.
Curious though for thoughts on this assumption.
There was a problem hiding this comment.
I think I like the idea of rather re-initializing the EM on refresh, so I'd rather stick to it. I suspect the transient state was never intended to be accesible from outside with set/put methods.
@ggershinsky let me know if you agree.
There was a problem hiding this comment.
Thanks @smaheshwar-pltr and @szlta , I also think it's cleaner to re-init the EM upon refresh.
| public void testRefresh() { | ||
| catalog.initialize(catalogName, catalogConfig); | ||
| Table table = catalog.loadTable(tableIdent); | ||
| validationCatalog.initialize(catalogName, catalogConfig); |
There was a problem hiding this comment.
Technically, the catalog -> validationCatalog isn't needed here, but figure it doesn't hurt to set things up for #13225.
| } | ||
|
|
||
| @TestTemplate | ||
| public void testConcurrentAppendTransactions() { |
There was a problem hiding this comment.
This tests an append retry
|
|
||
| // See CatalogTests#testConcurrentReplaceTransactions | ||
| @TestTemplate | ||
| public void testConcurrentReplaceTransactions() { |
There was a problem hiding this comment.
This tests a replace retry - note that replace retries don't re-apply updates to refreshed metadata, because they'll anyway replace the whole table. #13225 didn't handle this case at one point, because it requires keys to be maintained on refreshing different metadata, so I think valuable to test
|
cc @huaxingao @ggershinsky @szlta, LMKWYT - simplifying this code reduces duplication with the REST work in #13225. |
szlta
left a comment
szlta
left a comment
There was a problem hiding this comment.
Overall this looks great to me, thanks @smaheshwar-pltr
|
Thanks @smaheshwar-pltr for the PR! Thanks @ggershinsky @szlta for the review! |
|
Thanks all for the reviews 🙏 |
4 participants
While working on porting #14427 over to #13225, suspected that the flow could be simplified to handle the transaction cases.
This PR also adds tests inspired by the CatalogTests for these cases.