Conversation
zhjwpku
force-pushed
the
add_snapshot_update
branch
from
3c0ef7d to
5aff2f1
Compare
zhjwpku
force-pushed
the
add_snapshot_update
branch
from
5aff2f1 to
6c68501
Compare
zhjwpku
force-pushed
the
add_snapshot_update
branch
2 times, most recently
from
3c9c444 to
5e28bbb
Compare
zhjwpku
force-pushed
the
add_snapshot_update
branch
3 times, most recently
from
24d976d to
d5d0e20
Compare
| snapshot_id]() -> Result<std::unique_ptr<ManifestWriter>> { | ||
| std::string manifest_path = ManifestPath(); | ||
|
|
||
| if (format_version == 1) { |
There was a problem hiding this comment.
The related judgments for the format version (format_version) are scattered across multiple methods. Each version's logic exists in the form of if-else, encapsulating the processing logic of each version into independent implementations to avoid the expansion of if-else branches?
There was a problem hiding this comment.
I add MakeWriter helpers to wrap that, see #493
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
I haven't reviewed snapshot_update.h/.cc yet. Just post my findings so far.
zhjwpku
force-pushed
the
add_snapshot_update
branch
2 times, most recently
from
34a1fed to
1e47eef
Compare
wgtmac
force-pushed
the
add_snapshot_update
branch
from
1e47eef to
661e15a
Compare
wgtmac
force-pushed
the
add_snapshot_update
branch
from
661e15a to
1f42a1d
Compare
|
|
||
| const TableMetadata& Transaction::current() const { return metadata_builder_->current(); } | ||
|
|
||
| std::string Transaction::MetadataFileLocation(std::string_view filename) const { |
There was a problem hiding this comment.
It's also used in TableMetadataUtil::NewTableMetadataFilePath, I think it can be a public static method of TableMetadataUtil. Maybe do it in another PR.
dongxiao1198
left a comment
dongxiao1198
left a comment
There was a problem hiding this comment.
lgtm, this Finalize can be used in ExpireSnapshots also.
|
Thanks @zhjwpku for working on this large and complex feature and others for the review! Let me merge this to unblock other conflicting PRs. |
6 participants
No description provided.