Skip to content

HDDS-7566. Refactor TestRocksDBCheckpointDiffer tests.#8785

Merged
jojochuang merged 4 commits intoapache:masterfrom
jojochuang:HDDS-7566
Sep 23, 2025
Merged

HDDS-7566. Refactor TestRocksDBCheckpointDiffer tests.#8785
jojochuang merged 4 commits intoapache:masterfrom
jojochuang:HDDS-7566

Conversation

@jojochuang
Copy link
Contributor

What changes were proposed in this pull request?

HDDS-7566. Refactor TestRocksDBCheckpointDiffer tests.

Please describe your PR in detail:

  • The original issue meant to refactor the CompactionDag out of RocksDBCheckpointDiffer, which is already implemented. But the test is not.
  • Therefore, moved CompactionDag specific tests from TestRocksDBCheckpointDiffer to a new TestCompactionDag class. This makes the tests more organized and easier to maintain.

What is the link to the Apache JIRA

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

How was this patch tested?

mvn test -Dtest=TestCompactionDag,TestRocksDBCheckpointDiffer

Moved CompactionDag specific tests from TestRocksDBCheckpointDiffer to a new TestCompactionDag class.
This makes the tests more organized and easier to maintain.

Change-Id: I748810c0260f7baa974a4f9f9d8f3007d70f3c10
@jojochuang jojochuang requested review from sadanand48 and smengcl July 11, 2025 01:35
@jojochuang jojochuang added snapshot https://issues.apache.org/jira/browse/HDDS-6517 AI-gen labels Jul 11, 2025
@jojochuang
Copy link
Contributor Author

@smengcl @sadanand48 @SaketaChalamchala this is a trivial refactor.

@SaketaChalamchala
Copy link
Contributor

Thanks for the patch @jojochuang . We can also move testPruneOlderSnapshotsWithCompactionHistory to the TestCompactionDag class as it tests removing nodes (SST files) from the DAG that are older than configured time.

@smengcl smengcl requested a review from Copilot September 16, 2025 21:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors test organization by moving CompactionDag-specific tests from TestRocksDBCheckpointDiffer to a new dedicated TestCompactionDag class, improving test maintainability and organization.

  • Removes CompactionDag-related test constants, helper methods, and test cases from TestRocksDBCheckpointDiffer
  • Creates new TestCompactionDag class with moved CompactionDag-specific tests
  • Updates test methods to use CompactionDag instance instead of RocksDBCheckpointDiffer

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
TestRocksDBCheckpointDiffer.java Removes CompactionDag-specific test constants, helper methods, and test cases
TestCompactionDag.java New test class containing all moved CompactionDag-specific tests and utilities

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

return dag;
}

dag = GraphBuilder.directed().build();
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

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

Redundant variable assignment. The variable dag is already initialized on line 115 with the same value. Remove this duplicate assignment.

Suggested change
dag = GraphBuilder.directed().build();

Copilot uses AI. Check for mistakes.
}

/**
* Test cases for pruneBackwardDag.
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

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

The comment incorrectly describes test cases for pruneBackwardDag when this method provides test cases for pruneForwardDag. Update the comment to 'Test cases for pruneForwardDag.'

Suggested change
* Test cases for pruneBackwardDag.
* Test cases for pruneForwardDag.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

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

Thanks @jojochuang . I don't have more to add except pls address @SaketaChalamchala 's comment above.

Change-Id: I052727052e74a50b62e6e7752809007b18cd1897
Change-Id: Iadae0b62697dd67316a0de8557c8ab1ce5fedb85
Change-Id: Ib7b94d3e1a5c98aa6920f9504bb2c761cfe1a313
@jojochuang jojochuang merged commit 275b95c into apache:master Sep 23, 2025
28 checks passed
@jojochuang
Copy link
Contributor Author

Merged with Saketa's suggestion. Thanks @smengcl @SaketaChalamchala

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants