kbendick commented on a change in pull request #3801:
URL: https://github.com/apache/iceberg/pull/3801#discussion_r774843656
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/TestCachingCatalog.java
##########
@@ -252,9 +254,9 @@ public void
testCacheExpirationEagerlyRemovesMetadataTables() throws IOException
ticker.advance(HALF_OF_EXPIRATION);
Assertions.assertThat(catalog.cache().asMap()).doesNotContainKey(tableIdent);
- Arrays.stream(metadataTables(tableIdent)).forEach(metadataTable ->
- Assert.assertFalse("When a data table expires, its metadata tables
should expire regardless of age",
- catalog.cache().asMap().containsKey(metadataTable)));
+ // Removal of metadata tables from cache is async, use awaitility
+ await().untilAsserted(() ->
+
Assertions.assertThat(catalog.cache().asMap()).doesNotContainKeys(metadataTables(tableIdent)));
Review comment:
Side Note: Since you're introducing `Awaitility` (which in general I
think is a good idea), we should update the rest of the tests to use
`Awaitility` and remove the call to `cleanUp` in the `TestableCachingCatalog`,
which was added to handle the async expiration.
https://github.com/apache/iceberg/blob/9c3e340cd67699c3d2499762d794ab1bc1ee7f45/core/src/test/java/org/apache/iceberg/TestableCachingCatalog.java#L46-L51
This larger refactor to using `Awaitility` entirely should be done in a
separate PR though, as this fix is important and I'd like to get this PR in as
soon as possible so people can use the updated snapshot and we'll continue to
see if the race condition is fixed.
I was curious why this change was needed so I went looking through the
`Caffeine` issues and update notes and found the likely cause. Updating
Caffeine to [2.8.5](https://github.com/ben-manes/caffeine/releases/tag/v2.8.5)
would make this change not needed. The patch notes reference an issues that
says `"Fixed expiration delay for scheduled cleanup"` which is likely brought
in from the new / modified write path.
I also have another PR open to upgrade the `caffeine` library version, as
there are some important bug fixes for us and since we've been mucking around
in here, we might as well upgrade instead of stay behind.
https://github.com/apache/iceberg/pull/3803
All that said, I think the use of `Awaitility` here is fine. =)
Others might have different opinions based on a "smallest possible diff"
principal, where we'd introduce `Awaitility` in one PR by itself to help people
who backport, but I'm cool either way.
Thanks again for all your work on this!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
