Core: Clean expired metadata even if there is no snapshot to expire #13322
Conversation
| @@ -1799,6 +1799,33 @@ public void testExpireSnapshotsWithExecutor() { | |||
| .isGreaterThan(0); | |||
| } | |||
|
|
|||
| @TestTemplate | |||
| public void testRemoveMetadataWithNoSnapshots() throws Exception { | |||
There was a problem hiding this comment.
Could we have a test where there is no snapshots, and no metadata change either? Just to ensure that we do not fail?
There was a problem hiding this comment.
Thanks @lirui-apache I agree with the fix though it'd be good to have the additional test that @pvary mentioned.
|
Update to add test. Thanks @pvary @amogh-jahagirdar for reviewing |
| removeSnapshots(table) | ||
| .expireOlderThan(System.currentTimeMillis()) | ||
| .retainLast(1) | ||
| .cleanExpiredMetadata(true) | ||
| .commit(); | ||
| assertThat(table.ops().current()) | ||
| .as("No snapshot or metadata to remove, should be a no-op") | ||
| .isSameAs(current); |
There was a problem hiding this comment.
@lirui-apache I think I'd prefer to pull this out to a separate test testRemoveSnapshotsNoOp(), it's generally better to keep tests focused on a single aspect we're trying to test rather than have multiple assertions for different cases
There was a problem hiding this comment.
Sure, updated accordingly
There was a problem hiding this comment.
Thanks @lirui-apache this looks good to me now! I'll hold before merging in case @pvary has any other comments
|
Merged to main. |
|
Thanks @pvary @amogh-jahagirdar for reviewing and merging! |
This fixes #13294