danny0405 commented on code in PR #19985:
URL: https://github.com/apache/hudi/pull/19985#discussion_r4036085231
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiModule.java:
##########
@@ -75,7 +72,10 @@ public void configure(Binder binder)
binder.bind(FileFormatDataSourceStats.class).in(Scopes.SINGLETON);
newExporter(binder).export(FileFormatDataSourceStats.class).withGeneratedName();
- newOptionalBinder(binder,
CacheKeyProvider.class).setBinding().to(HudiCacheKeyProvider.class).in(Scopes.SINGLETON);
+ // HudiCacheKeyProvider is deliberately not bound on release-1.2.1, so
Trino's default
+ // provider is used. It implements the Trino 483 CacheKeyProvider
contract, which changed
Review Comment:
Could we add a regression test for updates to the newly cacheable
hoodie.properties and .index_defs/index.json files? The previous provider
explicitly bypassed them because they are mutable; after this change, freshness
depends on the default provider's lastModified/length key. A test that warms
the cache, replaces a metadata file at the same path, and verifies the next
read sees the replacement would cover the behavior described in the PR impact
section. Include a same-length replacement with a changed modification time so
the test checks timestamp invalidation too. The remaining Alluxio test only
looks for at least one cache hit and cannot establish metadata freshness.
##########
hudi-trino/src/test/java/io/trino/plugin/hudi/TestHudiMemoryCacheFileOperations.java:
##########
@@ -70,6 +71,7 @@ protected DistributedQueryRunner createQueryRunner()
}
@Test
+ @Disabled("Asserts the exclusions of HudiCacheKeyProvider in the metadata
memory cache (table properties and index definitions are re-read from storage
on every query). release-1.2.1 does not bind that provider, so Trino's default
provider caches those files and the counts no longer hold. Re-enable together
with the binding in HudiModule.")
Review Comment:
Could we update these assertions for the default provider, or retain a
focused memory-cache test, instead of disabling both tests in this class?
Together with the annotation on testJoin, this removes every test declared
here. The remaining testReadsServedFromAlluxioCache explicitly sets
hudi.metadata.cache.enabled=false, so it does not exercise this cache
implementation. Since metadata caching defaults to enabled, it would be useful
to retain a test with fs.cache.enabled=false that warms the memory cache and
verifies a subsequent read is served from it.
--
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]
