Skip to content

Conversation

@sadanand48
Copy link
Contributor

What changes were proposed in this pull request?

  • Makes the current test testContentsOfTarballWithSnapshot parameterized to cover both snapshot + non snapshot cases.
  • Test for batching logic in the transfer
    Existing tests in TestOMDBCheckpointServletInodeBasedXfer and TestOMRatisSnapshots cover this flow too.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13227

@sadanand48 sadanand48 added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Jul 31, 2025
@sadanand48 sadanand48 marked this pull request as ready for review July 31, 2025 15:15
Comment on lines +248 to +256
boolean obtainedFilesUnderMaxLimit = totalSize < maxFileSizeLimit;
if (!includeSnapshots) {
// If includeSnapshotData flag is set to false , it always sends all data
// in one batch and doesn't respect the max size config. This is how Recon
// uses it today.
assertFalse(obtainedFilesUnderMaxLimit);
} else {
assertTrue(obtainedFilesUnderMaxLimit);
}
Copy link
Contributor

@jojochuang jojochuang Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can simplify as:

Suggested change
boolean obtainedFilesUnderMaxLimit = totalSize < maxFileSizeLimit;
if (!includeSnapshots) {
// If includeSnapshotData flag is set to false , it always sends all data
// in one batch and doesn't respect the max size config. This is how Recon
// uses it today.
assertFalse(obtainedFilesUnderMaxLimit);
} else {
assertTrue(obtainedFilesUnderMaxLimit);
}
// If includeSnapshotData flag is set to false , it always sends all data
// in one batch and doesn't respect the max size config. This is how Recon
// uses it today.
assertEquals(includeSnapshots, totalSize < maxFileSizeLimit);

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. One small suggestion but it doesn't affect correctness.
Will merge later today.

@jojochuang jojochuang merged commit e515f3a into apache:master Jul 31, 2025
39 checks passed
jojochuang added a commit to jojochuang/ozone that referenced this pull request Nov 19, 2025
…pache#8884)"

This reverts commit e515f3a.

 Conflicts:
	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java

Change-Id: I5bbcc209483aca0343b713fe8dcc77dd0b3e7508
jojochuang added a commit to jojochuang/ozone that referenced this pull request Nov 20, 2025
…pache#8884)"

This reverts commit e515f3a.

 Conflicts:
	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java

Change-Id: I5bbcc209483aca0343b713fe8dcc77dd0b3e7508
jojochuang added a commit that referenced this pull request Nov 21, 2025
…8884)"

This reverts commit e515f3a.

 Conflicts:
	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServletInodeBasedXfer.java

Change-Id: I5bbcc209483aca0343b713fe8dcc77dd0b3e7508
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

snapshot https://issues.apache.org/jira/browse/HDDS-6517

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants