zhjwpku commented on code in PR #884:
URL: https://github.com/apache/iceberg-cpp/pull/884#discussion_r3774133877
##########
src/iceberg/update/merging_snapshot_update.h:
##########
@@ -60,12 +60,6 @@ namespace iceberg {
/// 5. Write new delete manifests (cached for commit retry)
/// 6. Merge data manifests (via data_merge_manager_)
/// 7. Merge delete manifests (via delete_merge_manager_)
-///
-/// TODO(Guotao): Java MergingSnapshotProducer overrides updateEvent() to
return a
Review Comment:
MetricsReporter covers the commit reporting use case, but it is not exactly
the same as Java's event listener mechanism. Java handles event notification
and metrics reporting separately. If we don't plan to support a general event
listener in C++, removing this TODO is fine.
##########
src/iceberg/test/merging_snapshot_update_test.cc:
##########
@@ -564,6 +581,29 @@ TEST_F(MergingSnapshotUpdateTest, CommitNewDataFile) {
EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedRecords), "100");
}
+TEST_F(MergingSnapshotUpdateTest, CommitReportsCreatedSnapshot) {
Review Comment:
+1 for this test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
