Skip to content

Conversation

@peterxcli
Copy link
Owner

@peterxcli peterxcli commented Nov 17, 2025

What changes were proposed in this pull request?

xxx

What is the link to the Apache JIRA

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

How was this patch tested?

(Please explain how this patch was tested. Ex: unit tests, manual tests, workflow run on the fork git repo.)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this.)


Note

Populate BlockLocation.topologyPaths in Ozone FS adapters and add tests verifying topology info for single- and multi-block files.

  • Adapters (ozonefs-common):
    • BasicOzoneClientAdapterImpl#getBlockLocations and BasicRootedOzoneClientAdapterImpl#getBlockLocations now collect datanode networkLocation and construct BlockLocation with names, hosts, and topologyPaths.
  • Tests (integration-test):
    • Add topology path assertions in AbstractOzoneFileSystemTest and AbstractRootedOzoneFileSystemTest:
      • Validate BlockLocation.getTopologyPaths() is non-null, has entries, and matches lengths of hosts/names.
      • Cover single- and multi-block files to ensure topology info is present across all blocks.

Written by Cursor Bugbot for commit 327d885. This will update automatically on new commits. Configure here.

@gemini-code-assist
Copy link

Summary of Changes

Hello @peterxcli, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces support for BlockLocation.topologyPaths within the Ozone file system. By integrating network topology information into BlockLocation objects, it enables applications to make more informed decisions about data placement and access, potentially leading to improved performance and resource utilization in distributed storage environments. The changes involve modifying the client adapters to retrieve and expose this information, along with adding new test cases to ensure its correctness.

Highlights

  • Topology Path Integration: BlockLocation objects now include network topology paths, providing more granular data location information for improved data access optimization.
  • Client Adapter Updates: The BasicOzoneClientAdapterImpl and BasicRootedOzoneClientAdapterImpl have been updated to extract and populate topologyPaths from datanode details when constructing BlockLocation objects.
  • Comprehensive Testing: New integration tests have been added to verify the correct functionality of BlockLocation.getTopologyPaths() for both single and multi-block files, ensuring the accuracy of the new topology information.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for BlockLocation.topologyPaths by populating this field when creating BlockLocation objects. The changes in BasicOzoneClientAdapterImpl and BasicRootedOzoneClientAdapterImpl correctly retrieve the network location from DatanodeDetails and pass it to the BlockLocation constructor. New integration tests are added to verify this functionality. The tests are generally good, but I have one suggestion to improve the completeness of a test case.

Comment on lines +2283 to +2286
String[] topologyPaths = blockLocation.getTopologyPaths();
assertNotNull(topologyPaths);
assertThat(topologyPaths.length).isGreaterThanOrEqualTo(1);
assertEquals(blockLocation.getHosts().length, topologyPaths.length);

Choose a reason for hiding this comment

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

medium

The assertions in this test for multi-block files are less comprehensive than in the single-block file test (testBlockLocationTopologyPaths). For better test coverage and consistency, I suggest adding assertions to check the length of topologyPaths against getNames().length and to verify that individual topology paths are not null, similar to the single-block test.

      String[] topologyPaths = blockLocation.getTopologyPaths();
      assertNotNull(topologyPaths);
      assertThat(topologyPaths.length).isGreaterThanOrEqualTo(1);
      assertEquals(blockLocation.getHosts().length, topologyPaths.length);
      assertEquals(blockLocation.getNames().length, topologyPaths.length);

      // Verify each topology path is not null
      for (String topologyPath : topologyPaths) {
        assertNotNull(topologyPath, "individual topology path should not be null");
      }

@github-actions
Copy link

github-actions bot commented Dec 9, 2025

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

@github-actions github-actions bot added the stale label Dec 9, 2025
@github-actions
Copy link

Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it.

@github-actions github-actions bot closed this Dec 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants