Add additional unit and integration tests for etag functionality #1972
Conversation
-Added a few corner case IT tests for testing etags with schema changes.
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @sandhyasun !
The tests looks very useful to me overall... just some minor comments.
service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| public void testETagBehaviorForTableDropAndRecreate() { |
There was a problem hiding this comment.
Is this test materially different from testETagChangeAfterMetadataLocationChange?
There was a problem hiding this comment.
Agreed - consolidated 3 tests into 1 that basically test metadata location change.
| // Verify all ETags are different from each other | ||
| for (int i = 0; i < allETags.size(); i++) { | ||
| for (int j = i + 1; j < allETags.size(); j++) { | ||
| assertThat(allETags.get(i)).isNotEqualTo(allETags.get(j)); |
There was a problem hiding this comment.
nit: might be more intuitive to put them all into a Set and check size.
There was a problem hiding this comment.
This 4th commit should show the change ? https://github.com/apache/polaris/pull/1972/commits/770b6bce05803f4bec45e61b38f437050986b023
There was a problem hiding this comment.
I still see nested loops 🤷 but it's a nit comment... approving.
|
|
||
| try (Response currentEtagResponse = currentEtagInvocation.invoke()) { | ||
| // Should return 304 Not Modified because ETag matches current version | ||
| assertThat(currentEtagResponse.getStatus()).isEqualTo(Response.Status.NOT_MODIFIED.getStatusCode()); |
There was a problem hiding this comment.
Would it also make sense to test this when v3ETag is combined with v1ETag?
There was a problem hiding this comment.
Added another case
|
|
||
| // Create a mock data file entry | ||
| DataFile dataFile = DataFiles.builder(table.spec()) | ||
| .withPath("/path/to/data/file1.parquet") |
There was a problem hiding this comment.
not sure this will work with cloud storage 🤔 Why not table.locationProvider().newDataLocation(...)?
There was a problem hiding this comment.
True ! Fixed it.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
2 participants
Description.
Motivation.
The additonal tests ensure that the ETag mechanism correctly handles the most common table changes - schema evolution and DDL/ DML operations. This ensures caching correctness in scenarios where tables are actively being written to and callers are caching etags for performance reasons
Related change
Related original change (Commit 8b5dfa9) (#1037)