feat: add SnapshotManager #542
Conversation
The test cases are derived from Java's TestSnapshotManager.java. Since MergeAppend is not supported yet, some tests are omitted for now and can be added later.
evindj
left a comment
evindj
left a comment
There was a problem hiding this comment.
Overall LGTM only question that I have is why is UpdateSnapshotReference and SnapShotManager both exposed. Also, is there a way we can integration test this change?
src/iceberg/update/snapshot_update.h
Outdated
| /// | ||
| /// \param branch Which is name of SnapshotRef of type branch | ||
| /// \return Reference to this for method chaining | ||
| auto& ToBranch(this auto& self, const std::string& branch) { |
There was a problem hiding this comment.
Should we rename SetTargetBranch to ToBranch as they are equivalent?
There was a problem hiding this comment.
To be precise, I was suggesting to remove ToBranch by reusing SetTargetBranch because they are literally equivalent. If we think ToBranch is a better name, we should just rename it without adding a new one.
There was a problem hiding this comment.
Got it, changed to SetTargetBranch.
src/iceberg/update/snapshot_update.h
Outdated
| /// | ||
| /// \param branch Which is name of SnapshotRef of type branch | ||
| /// \return Reference to this for method chaining | ||
| auto& ToBranch(this auto& self, const std::string& branch) { |
There was a problem hiding this comment.
To be precise, I was suggesting to remove ToBranch by reusing SetTargetBranch because they are literally equivalent. If we think ToBranch is a better name, we should just rename it without adding a new one.
| return *this; | ||
| } | ||
|
|
||
| SnapshotManager& SnapshotManager::SetCurrentSnapshot(int64_t snapshot_id) { |
There was a problem hiding this comment.
IIUC, we need to call CommitIfRefUpdatesExist() before anything in this SetCurrentSnapshot and RollbackToXXX below because different pending updates like UpdateSnapshotReference and SetSnapshot cannot be interleaved.
There was a problem hiding this comment.
Yes, the Java impl behaves this way, but I'm not entirely sure about our implementation. After calling CommitIfRefUpdatesExist(), the transaction state becomes committed, which should prevent any further commits.
There was a problem hiding this comment.
Maybe we can remove CommitIfRefUpdatesExist and put its logic directly into Commit.
There was a problem hiding this comment.
If we do not call CommitIfRefUpdatesExist to commit previous update, different updates in this SnapshotManager may see stale states. I see that the main issue is that Transaction::Commit() will be called automatically, is that correct?
| return *this; | ||
| } | ||
|
|
||
| SnapshotManager& SnapshotManager::SetCurrentSnapshot(int64_t snapshot_id) { |
There was a problem hiding this comment.
If we do not call CommitIfRefUpdatesExist to commit previous update, different updates in this SnapshotManager may see stale states. I see that the main issue is that Transaction::Commit() will be called automatically, is that correct?
3 participants
The test cases are derived from Java's TestSnapshotManager.java. Since MergeAppend is not supported yet, some tests are omitted for now and can be added later.