raunaqmorarka commented on code in PR #17164:
URL: https://github.com/apache/iceberg/pull/17164#discussion_r4060835176
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -520,6 +527,7 @@ public void commit() {
// to ensure that if a concurrent operation assigns the
UUID, this operation will
// not fail.
taskOps.commit(base, updated.withUUID());
+
metadataFileSizeInBytes.set(taskOps.metadataFileSizeInBytes());
Review Comment:
This reads a field shared by every thread committing through the same
TableOperations. If another thread's write lands between this thread's write
and its commit, the size reported here belongs to the other thread's metadata
file.
Suggest having the ops track (location, size) for its last write and only
accepting the value here when the location matches
`taskOps.current().metadataFileLocation()`. Otherwise leave it null.
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopTableOperations.java:
##########
@@ -152,7 +153,9 @@ public void commit(TableMetadata base, TableMetadata
metadata) {
TableMetadataParser.Codec codec =
TableMetadataParser.Codec.fromName(codecName);
String fileExtension = TableMetadataParser.getFileExtension(codec);
Path tempMetadataFile = metadataPath(UUID.randomUUID() + fileExtension);
- TableMetadataParser.write(metadata,
io().newOutputFile(tempMetadataFile.toString()));
+ this.metadataFileSizeInBytes =
Review Comment:
This is set before `renameToFinal`, so a failed rename leaves the size of a
file that never became current. Set it after the rename succeeds.
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -520,6 +527,7 @@ public void commit() {
// to ensure that if a concurrent operation assigns the
UUID, this operation will
// not fail.
taskOps.commit(base, updated.withUUID());
+
metadataFileSizeInBytes.set(taskOps.metadataFileSizeInBytes());
Review Comment:
Inside a transaction `taskOps` is `TransactionTableOperations`, which does
not write a metadata file, so this is always null there. The file is written
later in `BaseTransaction` with no report. Either plumb the size through the
transaction commit or call this out in the javadoc and docs.
##########
core/src/main/java/org/apache/iceberg/TableOperations.java:
##########
@@ -127,4 +127,17 @@ default long newSnapshotId() {
default boolean requireStrictCleanup() {
return true;
}
+
+ /**
+ * Returns the size in bytes of the metadata file written for the most
recent commit handled by
+ * this table operations instance.
+ *
+ * <p>This value is optional and may be unavailable for implementations that
do not write metadata
+ * files directly or cannot determine the final stored length at write time.
+ *
+ * @return metadata file size in bytes, or null if unavailable
+ */
+ default Long metadataFileSizeInBytes() {
Review Comment:
"Most recent commit handled by this instance" is not what callers get under
concurrent commits or transactions. Please tighten the contract to the metadata
file at `current().metadataFileLocation()`, and return null when that was not
written by this instance.
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -508,4 +512,67 @@ public void close() throws Exception {}
@Override
public void initialize(Map<String, String> properties) {}
}
+
+ @Test
+ public void commitReportContainsMetadataFileSizeInBytes() {
Review Comment:
Only `HadoopTableOperations` is covered. `BaseMetastoreTableOperations` is
the path every real catalog uses. Please add an equivalent test through
`InMemoryCatalog`.
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -508,4 +512,67 @@ public void close() throws Exception {}
@Override
public void initialize(Map<String, String> properties) {}
}
+
+ @Test
+ public void commitReportContainsMetadataFileSizeInBytes() {
+ BaseTable baseTable = (BaseTable) table;
+ HadoopTableOperations ops = (HadoopTableOperations) baseTable.operations();
+
+ AtomicReference<CommitReport> capturedReport = new AtomicReference<>();
+ MetricsReporter reporter =
+ report -> {
+ if (report instanceof CommitReport) {
+ capturedReport.set((CommitReport) report);
+ }
+ };
+
+ Table tableWithReporter = new BaseTable(ops, baseTable.name(), reporter);
+ tableWithReporter.newFastAppend().appendFile(FILE_A).commit();
+
+ CommitReport commitReport = capturedReport.get();
+ assertThat(commitReport).isNotNull();
+
assertThat(reportedMetadataFileSize(commitReport)).isEqualTo(metadataFileSize(ops));
+ }
+
+ @Test
+ public void eachCommitReportsTheMetadataFileSizeItWrote() {
Review Comment:
Please also cover a retried commit: first attempt fails with
`CommitFailedException`, second succeeds, and the report carries the second
file's size. The concurrent commit helpers in this class should make that
straightforward.
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -508,4 +512,67 @@ public void close() throws Exception {}
@Override
public void initialize(Map<String, String> properties) {}
}
+
+ @Test
+ public void commitReportContainsMetadataFileSizeInBytes() {
+ BaseTable baseTable = (BaseTable) table;
+ HadoopTableOperations ops = (HadoopTableOperations) baseTable.operations();
+
+ AtomicReference<CommitReport> capturedReport = new AtomicReference<>();
+ MetricsReporter reporter =
+ report -> {
+ if (report instanceof CommitReport) {
+ capturedReport.set((CommitReport) report);
+ }
+ };
+
+ Table tableWithReporter = new BaseTable(ops, baseTable.name(), reporter);
+ tableWithReporter.newFastAppend().appendFile(FILE_A).commit();
+
+ CommitReport commitReport = capturedReport.get();
+ assertThat(commitReport).isNotNull();
+
assertThat(reportedMetadataFileSize(commitReport)).isEqualTo(metadataFileSize(ops));
+ }
+
+ @Test
+ public void eachCommitReportsTheMetadataFileSizeItWrote() {
Review Comment:
Missing a transaction case. That would have caught the null result from
`TransactionTableOperations`.
##########
core/src/test/java/org/apache/iceberg/metrics/TestCommitReportParser.java:
##########
@@ -300,4 +300,28 @@ public void roundTripSerdeWithMetadata() {
assertThat(CommitReportParser.fromJson(json)).isEqualTo(commitReport);
assertThat(json).isEqualTo(expectedJson);
}
+
+ @Test
+ public void roundTripSerdeWithMetadataFileSizeInCommitMetrics() {
+ String tableName = "roundTripTableName";
+ CommitReport commitReport =
+ ImmutableCommitReport.builder()
+ .tableName(tableName)
+ .snapshotId(23L)
+ .operation("DELETE")
+ .sequenceNumber(4L)
+ .commitMetrics(
+ ImmutableCommitMetricsResult.builder()
+
.metadataFileSizeInBytes(CounterResult.of(MetricsContext.Unit.BYTES, 123L))
+ .build())
+ .build();
+
+ String json = CommitReportParser.toJson(commitReport, true);
+
+ assertThat(json)
Review Comment:
Same here, compare the full expected JSON like the sibling tests.
##########
core/src/test/java/org/apache/iceberg/metrics/TestCommitMetricsResultParser.java:
##########
@@ -271,4 +272,20 @@ public void roundTripSerdeNoopCommitMetrics() {
assertThat(CommitMetricsResultParser.fromJson(json))
.isEqualTo(ImmutableCommitMetricsResult.builder().build());
}
+
+ @Test
+ public void roundTripSerdeWithMetadataFileSizeInBytes() {
+ CommitMetricsResult result =
+ ImmutableCommitMetricsResult.builder()
+
.metadataFileSizeInBytes(CounterResult.of(MetricsContext.Unit.BYTES, 123L))
+ .build();
+
+ String json = CommitMetricsResultParser.toJson(result, true);
+
+ assertThat(json)
Review Comment:
The other tests in this file compare against the exact expected JSON. Please
do the same here so field placement and formatting are pinned.
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -508,4 +512,67 @@ public void close() throws Exception {}
@Override
public void initialize(Map<String, String> properties) {}
}
+
+ @Test
+ public void commitReportContainsMetadataFileSizeInBytes() {
Review Comment:
Please add a case with `write.metadata.compression-codec=gzip`. That is the
case where reading the length only after close matters, and nothing exercises
it.
##########
core/src/test/java/org/apache/iceberg/TestCommitReporting.java:
##########
@@ -90,6 +90,7 @@ public void addAndDeleteDataFiles() {
assertThat(metrics.manifestsCreated().value()).isEqualTo(1L);
assertThat(metrics.manifestsKept().value()).isEqualTo(0L);
assertThat(metrics.manifestsReplaced().value()).isEqualTo(1L);
+ assertThat(metrics.metadataFileSizeInBytes()).isNull();
Review Comment:
These `isNull` asserts only confirm `TestTables` reports nothing. They do
not verify the feature. Consider dropping them, or move the positive assertions
here once a metastore-backed test exists.
##########
docs/docs/metrics-reporting.md:
##########
@@ -41,6 +41,7 @@ A
[`CommitReport`](https://github.com/apache/iceberg/blob/main/core/src/main/jav
* number of added/removed data/delete files
* number of added/removed equality/positional delete files
* number of added/removed equality/positional deletes
+* size in bytes of the table metadata file written by the commit, when
available (some catalogs do not write metadata files directly)
Review Comment:
Also mention that commits made through a `Transaction` do not report this,
unless that gets fixed in this PR.
--
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]
