kbendick commented on a change in pull request #3801:
URL: https://github.com/apache/iceberg/pull/3801#discussion_r774898150
##########
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:
Thinking on it further, **Can we upgrade Caffeine** to the next minor
version (2.8.5) in this PR, which makes this `Awaitility` introduction
unnecessary?
My reasoning is that the bug that causes you to have to use `Awaitility`
here, is fixed byy upgrading to Caffeine 2.8.5. This indicates that the write
path we're going through _also_ receives the fix and so we should provide that
fix to our actual code too (even if we're not explicitly calling `cleanUp`
ever, the bug could still affect us).
It's the[ only patch mentioned in the release
notes](https://github.com/ben-manes/caffeine/releases/tag/v2.8.5) for 2.8.4 ->
2.8.5
We can and should still convert this to not call `cleanUp` in the
`TestableCachingCache`, but I think it's best that the fix from 2.8.5 is also
applied to the final code as it's a scheduling bug that could affect our new
write path for this cache as well. This way also, we can introduce `Awaitility`
in a follow up PR and focus only on this bug.
Also, when we upgrade Caffeine to 2.9.x or 3.0 (same release more or less),
we should look into changing this to use the newly added `.evictionListener`,
which would make the metadata table drops fully atomic. It might make the
`Awaitility` changes not fully needed. But my main thinking is upgrade to
caffeine 2.8.5 for now to get the scheduling fix that we know otherwise likely
affects us.
--
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]
