fix: handle missing parent snapshots in ExpireSnapshots #671
Conversation
After snapshot expiration, remaining snapshots may have parent-snapshot-id references pointing to snapshots that no longer exist. ExpireSnapshots was failing when walking the parent chain and encountering these dangling references. Change the behavior to treat a missing parent snapshot as the end of the chain rather than returning an error. This is expected and valid behavior in Iceberg tables that have undergone previous snapshot expiration.
| if err != nil { | ||
| return err | ||
| // Parent snapshot may have been removed by a previous expiration. | ||
| // Treat missing parent as end of chain - this is expected behavior. | ||
| break |
There was a problem hiding this comment.
Should we match on the specific error type?
There was a problem hiding this comment.
@twuebi not sure I understand. There's really only one reason SnapshotByID() fails -- when the given snapshot ID doesn't exist in snapshotList list.
There was a problem hiding this comment.
probably goes beyond the scope of this PR, if the only reason for SnapshotByID to ever error is not found, then it should probably become a bool, ptr or simply just ptr interface.
Relying on the function to only return an err for not found means that we rely on something that's not statically checked, although unlikely, someone may change SnapshotByID to return err on other cases too, and we'd treat that as happy path here.
|
Thanks @dhananjaykrutika can you add a test for this? |
3 participants
After snapshot expiration, remaining snapshots may have parent-snapshot-id references pointing to snapshots that no longer exist. ExpireSnapshots was failing when walking the parent chain and encountering these dangling references.
Change the behavior to treat a missing parent snapshot as the end of the chain rather than returning an error. This is expected and valid behavior in Iceberg tables that have undergone previous snapshot expiration.