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?
…SnapshotManager::Commit
zhjwpku
force-pushed
the
add_snapshot_manager
branch
from
0ae7a0d to
db23355
Compare
|
|
||
| private: | ||
| friend class PendingUpdate; | ||
| friend class SnapshotManager; |
There was a problem hiding this comment.
Yeah, this is necessary, because NewSetSnapshot and NewUpdateSnapshotReference have been moved to private.
| } | ||
|
|
||
| Result<std::shared_ptr<SnapshotManager>> Transaction::NewSnapshotManager() { | ||
| // SnapshotManager has its own commit logic, so it is not added to the pending updates. |
There was a problem hiding this comment.
We can still call AddUpdate just like other updates and do not set last_update_committed_ to false for SnapshotManager. However, it seems that SnapshotManager cannot be retried easily so I'm fine to keep the current approach as is.
src/iceberg/table.cc
Outdated
| Result<std::shared_ptr<SnapshotManager>> Table::NewSnapshotManager() { | ||
| ICEBERG_ASSIGN_OR_RAISE( | ||
| auto transaction, Transaction::Make(shared_from_this(), Transaction::Kind::kUpdate, | ||
| /*auto_commit=*/false)); |
There was a problem hiding this comment.
Here auto_commit means that users do not need to call Transaction::Commit() after calling SnapshotManager::Commit(). Should we set this to true and handle the commit logic internally?
| } | ||
|
|
||
| Status SnapshotManager::Commit() { | ||
| transaction_->EnableAutoCommit(); |
There was a problem hiding this comment.
I think we need to read and backup Transaction::auto_commit_ in the SnapshotManager ctor and then restore it before exiting this function, otherwise any CommitIfRefUpdatesExist() before SnapshotManager::Commit() will set Transaction::committed_ to true.
There was a problem hiding this comment.
To verify this issue, we can create a SnapshotManager from Table & Transaction and then call SetCurrentSnapshot and RollbackXXX for multiple times to see it they can be committed successfully.
There was a problem hiding this comment.
Test cases have been added, and I also implemented the backup/restore logic. I'm a bit unsure why this is necessary, after SnapshotManager::Commit(), the transaction is already marked as committed, I'm not clear what auto_commit_ is intended for after the commit.
src/iceberg/test/update_test_base.h
Outdated
| void SetUp() override { | ||
| InitializeFileIO(); | ||
| RegisterTableFromResource("TableMetadataV2Valid.json"); | ||
| RegisterMinimalTableFromResource("TableMetadataV2ValidMinimal.json"); |
There was a problem hiding this comment.
It looks weird to register two resources in the per-case setup function but each case uses only one of them. Does it make sense to use class inheritance for each resource?
There was a problem hiding this comment.
Make sense, I've split it into two classes: UpdateTestBase/MinimalUpdateTestBase.
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.