HDDS-13006. Use yaml files to host Ozone snapshot local properties

[jojochuang]

What changes were proposed in this pull request?

Use one .yaml file for each Ozone snapshot DB to store local properties for each Ozone snapshop DB checkpoint. e.g. isSstFiltered

What is the link to the Apache JIRA

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

How was this patch tested?

• TBD

[gemini-code-assist]

Hello @jojochuang, I'm Gemini Code Assist¹! 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!

Summary of Changes

Gemini here, providing a summary for this pull request. Based on the title and description, the primary goal of this PR is to introduce the use of YAML files to store local properties for Ozone snapshot DB checkpoints. This allows for per-OM properties, such as the isSstFiltered flag, to be stored alongside the snapshot checkpoint data. However, the patches included in this PR also contain several other distinct changes, including enhancements to the Container Balancer configuration validation, additions to the Freon random key generator tool, improvements to the ozone debug replicas verify command, and refactoring within the Ozone Manager's KeyManagerImpl for handling pending deletions.

Highlights

• Snapshot Local Properties: Introduces an interface (OmSnapshotLocalPropertyManager) and a helper method (OmSnapshotManager.getSnapshotLocalPropertyPath) to manage per-OM local properties for snapshot DB checkpoints using YAML files.
• Container Balancer Validation: Adds a new validation rule in ContainerBalancer to ensure that hdds.container.balancer.move.timeout minus hdds.container.balancer.move.replication.timeout and hdds.scm.replication.event.timeout.datanode.offset is greater than or equal to 9 minutes. A corresponding test case is added.
• Freon Random Key Generator Enhancement: Adds a --bucket-layout option to the RandomKeyGenerator tool, allowing users to specify the bucket layout (e.g., OBJECT_STORE, FILE_SYSTEM_OPTIMIZED) when creating buckets. Adds a test for this new option.
• Ozone Debug Replicas Verify Enhancement: Adds a new verifier (ContainerStateVerifier) to the ozone debug replicas verify command to check the state of containers and their replicas on datanodes and compare it with the state in SCM. Introduces a --container-state flag to enable this check and a --container-cache-size option for caching container info.
• OM KeyManagerImpl Refactoring: Refactors methods related to retrieving pending deletion entries (getPendingDeletionKeys, getPendingDeletionSubDirs, getPendingDeletionSubFiles) in KeyManagerImpl to use a common helper method (gatherSubPathsWithIterator) and standardizes the use of Table.KeyValue vs KeyValue imports.

Changelog

Click here to see the changelog

• hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
  • Added imports for Duration and TimeUnit (lines 22, 24).
  • Added a new validation check for balancer timeouts (lines 465-480).
• hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java
  • Added a test case to verify the new balancer timeout validation rule (lines 208-226).
• hadoop-ozone/dist/src/main/smoketest/debug/ozone-debug-tests.robot
  • Removed unused variable ${OM_SERVICE_ID} (line 30).
  • Modified ozone debug replicas verify command to include --block-existence and --container-state flags (line 40).
• hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/RandomKeyGenerator.java
  • Added imports for BucketArgs and BucketLayout (lines 63, 71).
  • Added --bucket-layout command line option (lines 196-200).
  • Added getBucketMapSize method for testing (lines 657-665).
  • Modified createBucket to use the specified bucket layout if provided (lines 778-785).
  • Made getBucket method visible for testing (line 909).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestContainerBalancerOperations.java
  • Adjusted moveReplicationTimeout value in testContainerBalancerCLIOperations (line 85).
  • Adjusted moveReplicationTimeout value in testIfCBCLIOverridesConfigs (line 150).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/freon/TestRandomKeyGenerator.java
  • Added imports for OzoneBucket and BucketLayout (lines 24, 25).
  • Added testBucketLayoutOption to verify the new bucket layout functionality (lines 191-210).
• hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugShell.java
  • Added --container-state flag to the replicas verify command in testReplicasVerifyCmd (line 106).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
  • Added imports for Table.KeyValue and WithParentObjectId (lines 134, 164).
  • Changed type parameters from Table.KeyValue to KeyValue in getPendingDeletionKeys (lines 739, 747, 753, 765).
  • Changed type parameters from Table.KeyValue to KeyValue in getTableEntries (lines 800, 801, 805, 816).
  • Changed type parameters from Table.KeyValue to KeyValue in getRenamesKeyEntries (lines 838, 840, 843).
  • Changed type parameters from Table.KeyValue to KeyValue in getDeletedKeyEntries (lines 887, 889, 892).
  • Changed type parameters from Table.KeyValue to KeyValue in getDeletedDirEntries (line 2192).
  • Refactored getPendingDeletionSubDirs to use gatherSubPathsWithIterator (lines 2198-2203).
  • Added generic helper method gatherSubPathsWithIterator (lines 2205-2238).
  • Refactored getPendingDeletionSubFiles to use gatherSubPathsWithIterator (lines 2240-2247).
  • Changed type parameters from Table.KeyValue to KeyValue in listStatus (line 1856).
  • Changed type parameters from Table.KeyValue to KeyValue in getIteratorForKeyInTableCache (lines 1913, 1918).
  • Changed type parameters from Table.KeyValue to KeyValue in findKeyInDbWithIterator (lines 1936, 1941).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java
  • Added new interface OmSnapshotLocalPropertyManager for managing snapshot local properties (lines 1-88).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
  • Added static helper method getSnapshotLocalPropertyPath (lines 767-770).
• hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java
  • Added helper method getKeyInfoWithFullPath for OmDirectoryInfo (lines 723-729).
  • Added helper method getKeyInfoWithFullPath for OmKeyInfo (lines 731-736).
• hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/replicas/BlockExistenceVerifier.java
  • Added setId(datanode.getID()) when building the pipeline (line 59).
• hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/replicas/ContainerStateVerifier.java
  • Added new class ContainerStateVerifier to check container and replica states (lines 1-194).
• hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/replicas/ReplicasVerify.java
  • Added --container-cache-size option (lines 70-76).
  • Added ContainerStateVerifier if --container-state is enabled (lines 91-93).
  • Added doExecuteReplicaState flag to Verification inner class (lines 236-241).
• hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/replicas/chunk/ChunkKeyHandler.java
  • Changed JSON key name from ecIndex to replicaIndex (line 199).

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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

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 issue 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 is currently in preview and 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 to provide feedback.

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.

---

Code review time is here,
Changes flow, make logic clear.
Fix the bugs, add new features,
Build a system that endures.

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. ↩

[gemini-code-assist]

Code Review

This pull request introduces the OmSnapshotLocalPropertyManager interface as per HDDS-13006, and also includes several other enhancements and refactorings across ContainerBalancer, Freon, Ozone debug tools, and KeyManagerImpl. The changes are generally well-implemented.

Key highlights:

• The refactoring in KeyManagerImpl to consolidate logic for deleting sub-paths is a good improvement for maintainability.
• The new ContainerStateVerifier enhances the capabilities of the ozone debug replicas verify tool.
• The addition of bucket layout support in Freon is useful for testing.

Please ensure that the testing (marked as TBD in the PR description) is completed and documented. There are a couple of minor points to address, detailed in the specific comments.

Summary of Findings

• Magic Number in ContainerBalancer: A magic number (9 minutes) is used in ContainerBalancer validation. Consider making it a named constant or configurable for better clarity and maintainability.
• Logging Practice in ContainerStateVerifier: System.err.println is used in ContainerStateVerifier for an invalid cache size warning. It's recommended to use an SLF4J logger for consistency and better log management.
• Testing Status: The PR description indicates that testing is 'TBD'. Comprehensive testing is essential before merging.
• Code Refactoring: The refactoring in KeyManagerImpl to use gatherSubPathsWithIterator is a positive change, reducing code duplication.
• New Features: The new ContainerStateVerifier and Freon's bucket layout support are valuable additions.
• Test Configuration: The test configuration in TestContainerBalancerOperations.java will now throw an exception due to the new validation logic. Consider updating the test configuration to be valid.

Merge Readiness

The pull request introduces valuable features and refactorings. However, the testing status is 'TBD', and there are a couple of medium-severity issues (related to a magic number and test configuration) and a low severity issue (related to logging practice) that should be addressed. Once these points are resolved and testing is completed, the PR should be in good shape for merging. As an AI, I cannot approve PRs; please ensure human reviewers provide final approval.

[gemini-code-assist]

The magic number 9 (minutes) is used here as a threshold. Could this be defined as a named constant (e.g., MINIMUM_TIME_BUFFER_FOR_MOVE_OPERATIONS_MINUTES = 9) at the class or configuration level? This would improve readability and maintainability, especially if this value has specific significance or might need adjustment in the future.

For example:

private static final Duration MINIMUM_OPERATION_BUFFER_DURATION = Duration.ofMinutes(9);
// ...
if ((conf.getMoveTimeout().toMillis() - conf.getMoveReplicationTimeout().toMillis() - datanodeOffset) 
    < MINIMUM_OPERATION_BUFFER_DURATION.toMillis()) {
  // ...
}

private static final Duration MINIMUM_OPERATION_BUFFER_DURATION = Duration.ofMinutes(9);

[gemini-code-assist]

The move replication timeout is set to 50 minutes, while the move timeout is set to 65 minutes. Given the new validation logic added to ContainerBalancer.java, this configuration will now throw an InvalidContainerBalancerConfigurationException. It would be good to update this test to ensure the configuration is valid, or loosen the validation to allow for this case.

Style Guide References

[jojochuang]

/gemini review

[github-actions]

No such command. gemini Available commands:

• /close : Close pending pull request temporary
• /help : Show all the available comment commands
• /label : add new label to the issue: /label <label>
• /pending : Add a REQUESTED_CHANGE type review to mark issue non-mergeable: /pending <reason>
• /ready : Dismiss all the blocking reviews by github-actions bot
• /retest : provide help on how to trigger new CI build

[gemini-code-assist]

Code Review

This pull request introduces a significant number of unrelated changes, making it difficult to review and increasing the risk of introducing unintended issues. The PR's title and description state its purpose is to 'Use yaml files to host Ozone snapshot local properties', but it also includes:

• Major documentation updates and new design documents for unrelated features (e.g., Full Volume Handling, S3 API documentation).
• Refactoring of CLI command options into a mixin.
• Changes to test scripts and test infrastructure (docker-compose, robot framework).
• Changes to Recon, including UI updates.
• Changes to byteman integration.

It is highly recommended to split these unrelated changes into separate pull requests. This will make each PR smaller, more focused, and easier to review and merge.

My review focuses on the core changes related to the snapshot YAML properties, but I've also included a few comments on the unrelated changes to highlight the issue.

[gemini-code-assist]

The setLastCompactionTime method accepts a Long object, but the lastCompactionTime field is a primitive long. If a null value is passed to this method, it will result in a NullPointerException when unboxing. To prevent this, the parameter type should be changed to the primitive long.

Suggested change

	public void setLastCompactionTime(Long lastCompactionTime) {
	this.lastCompactionTime = lastCompactionTime;
	}
	public void setLastCompactionTime(long lastCompactionTime) {
	this.lastCompactionTime = lastCompactionTime;
	}

[gemini-code-assist]

This new design document for 'Full Volume Handling' (JIRA HDDS-12929) appears to be unrelated to the main goal of this pull request, which is about using YAML files for snapshot properties (JIRA HDDS-13006). Please move this and other unrelated documentation changes to a separate pull request to keep this one focused.

[gemini-code-assist]

This change seems unrelated to the main purpose of this pull request (snapshot properties in YAML). It would be better to move this and other related CLI refactoring changes to a separate pull request for better focus and reviewability.

Additionally, the chained ternary operator can be difficult to read. A series of if statements would improve readability.

  public String getNodeId() {
    if (!Strings.isNullOrEmpty(selection.nodeId)) {
      return selection.nodeId;
    }
    if (!Strings.isNullOrEmpty(selection.id)) {
      return selection.id;
    }
    if (!Strings.isNullOrEmpty(selection.uuid)) {
      return selection.uuid;
    }
    return "";
  }

[gemini-code-assist]

This Javadoc comment contains a TODO to clarify the ver parameter. This should be resolved before merging to improve code clarity and maintainability.

[gemini-code-assist]

The logic for creating a Yaml instance is duplicated from the getYamlForSnapshotLocalData() method. To avoid code duplication and improve maintainability, this method should be simplified to call getYamlForSnapshotLocalData() directly.

  public static OmSnapshotLocalDataYaml getFromYamlStream(InputStream input) throws IOException {
    OmSnapshotLocalDataYaml dataYaml;

    final Yaml yaml = getYamlForSnapshotLocalData();

    try {
      dataYaml = yaml.load(input);
    } catch (YAMLException ex) {
      // Unchecked exception. Convert to IOException
      throw new IOException(ex);
    }

    if (dataYaml == null) {
      // If Yaml#load returned null, then the file is empty. This is valid yaml
      // but considered an error in this case since we have lost data about
      // the snapshot.
      throw new IOException("Failed to load snapshot file. File is empty.");
    }

    return dataYaml;
  }