adutra commented on PR #15192: URL: https://github.com/apache/iceberg/pull/15192#issuecomment-3828941035
> Shall we add a test which fails without the change? Otherwise what would prevent someone to break this again for Polaris? I think the proposed fix is enough and is the best option. Let me explain the issue differently: When this test is executed with an Apache Polaris server: 1. The metadata file is deleted manually 2. `RESTCatalog.loadTable` is called 3. The `RESTCatalog` client sends a request with an `IfNoneMatch` header 4. The server loads the table metadata from its cache **without hitting the storage layer** 5. The server sends back a 304 (Not Changed) response 6. The `RESTCatalog` client returns the cached `Table` instance 7. The test fails. This issue doesn't happen in Iceberg REST tests because: 1. The test was overridden in `TestRESTCatalog` to use `InMemoryFileIO` to delete the file 2. `RESTCatalogAdapter` loads the table using the same `InMemoryFileIO` and thus "notices" the file deletion 3. The test passes. There is imho a fundamental problem in the test since it relies on the fact that the `loadTable` call will NOT be cached, and will NOT be served from memory. But this assumption relies on implementation details that differ from catalog to catalog, thus making the test outcome unreliable. -- 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]
