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.
The use of awaitility is fine here though, but I was curious why it 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 version bump says `Fixed expiration delay for scheduled cleanup` which
must be why this test started failing with the new write path into the cache. I
only mention it as I was very curious why this particular change was needed
given the existing calls to `cleanUp`.
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]
