rdblue commented on a change in pull request #1991:
URL: https://github.com/apache/iceberg/pull/1991#discussion_r549450197
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -239,8 +239,10 @@ public void testMergeWithExistingManifest() {
public void testManifestMergeMinCount() throws IOException {
Assert.assertEquals("Table should start empty", 0,
listManifestFiles().size());
table.updateProperties().set(TableProperties.MANIFEST_MIN_MERGE_COUNT, "2")
- // each manifest file is 5227 bytes, so 12000 bytes limit will give us
2 bins with 3 manifest/data files.
- .set(TableProperties.MANIFEST_TARGET_SIZE_BYTES, "12000")
+ // Each initial v1/v2 ManifestFile is 5661/6397 bytes respectively.
Merging two of the given
+ // manifests make one v1/v2 ManifestFile of 5672/6408 bytes
respectively, so 12850 bytes
+ // limit will give us two bins with three manifest/data files.
+ .set(TableProperties.MANIFEST_TARGET_SIZE_BYTES, "12850")
Review comment:
3x the smaller size would be around 17k, and we need it to be at least
about 13k, which is 2x the larger size. I'd probably set this to 15k to split
the difference and hopefully avoid needing to update this again as tests
change. This is minor, though.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
