feat(restore-indices): add additional step to also clear system metadata service

[Masterchen09]

Not directly connected, but theses changes were also the result of this PR in the datahub-helm repository (see my thoughts there for context and more information): acryldata/datahub-helm#475

If we say, that the database is the single-point-of-truth, then the SystemMetadataService also has to be cleaned-up, when the other search indices/services (i.e. EntitySearchService and GraphService) are cleaned-up. Otherwise there could be entries left in the search index, which are not present in the database anymore.

As far as I understand, the UpdateIndicesService, which is used by the MAE consumer, is always updating the SystemMetadataService (similar to the EntitySearchService and GraphService in case of the restore-indices job), therefore it should be save to also clean-up the SystemMetadataService before resp. to have a consistent result it must be cleaned-up (I am not sure for what the system metadata is used, however if one service is cleaned-up the others should be cleaned-up too?).

I am not sure regarding the removal of the @VisibleForTesting annotation - I am not an experienced Java developer 😉 - the "clear" method of the ElasticSearchGraphService and ElasticSearchSystemMetadataService are not only used for testing, therefore those methods are not only visible for testing...additionally and the interface which defines the methods does not have this annotation, therefore implementing classes should also not have this annotation?

Checklist

• The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• Links to related issues (if applicable)
• Tests for the changes have been added/updated (if applicable)
• Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
• For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

Summary by CodeRabbit

• New Features

  • Introduced a step to clear system metadata during upgrades, enhancing data maintenance.

• Refactor

  • Renamed internal service clients for consistency and clarity.
  • Adjusted configurations to integrate SystemMetadataService.

• Style

  • Removed @VisibleForTesting annotation from clear() methods to reflect updated visibility.

[jjoyce0510]

This looks good to me, just pending some review from the team.

Only slight hesitation is that i think Restore Incidents allows filtering by different things (e.g. entity type), which would not be honored when clearing the system metadata index.

I do agree that this is useful to achieve consistency between primary DB and Elastic Indices.

[jjoyce0510]

Apologies, I see that we are already doing this in the search and graph step based on the same args context. We should be good on this. Removing @VisibleForTesting now that we are using the "clear()" method makes sense

[jjoyce0510]

Unclear why so many smoke tests are failing. Trying to force run them again.

[coderabbitai]

Walkthrough

This update introduces a new ClearSystemMetadataServiceStep class to manage system metadata during upgrades, along with renaming fields and updating method calls in existing upgrade steps (GMSDisableWriteModeStep and GMSEnableWriteModeStep). Dependencies on SystemMetadataService are added to RestoreBackup and RestoreIndices configurations and steps. Additionally, the visibility of certain methods related to system metadata handling in ElasticSearchGraphService and ElasticSearchSystemMetadataService has been adjusted for broader usage beyond testing.

Changes

File Path	Change Summary
.../ClearSystemMetadataServiceStep.java	Introduced new class for clearing system metadata as an upgrade step.
.../GMSDisableWriteModeStep.java	Renamed entityClient to systemEntityClient and updated method calls accordingly.
.../GMSEnableWriteModeStep.java	Renamed entityClient to systemEntityClient and updated method calls accordingly.
.../RestoreBackupConfig.java	Added SystemMetadataService dependency in createInstance method.
.../RestoreIndicesConfig.java	Added SystemMetadataService dependency in createInstance method.
.../RestoreBackup.java	Added ClearSystemMetadataServiceStep, renamed entityClient and searchClient parameters in constructor and method calls.
.../RestoreIndices.java	Updated constructor and method calls to include SystemMetadataService.
.../ElasticSearchGraphService.java	Removed @VisibleForTesting annotation from clear() method.
.../ElasticSearchSystemMetadataService.java	Removed @VisibleForTesting annotation from clear() method.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant RestoreBackup as RestoreBackup
    participant GMSDisableWriteModeStep as GMSDisableWriteModeStep
    participant ClearSystemMetadataServiceStep as ClearSystemMetadataServiceStep
    participant GMSEnableWriteModeStep as GMSEnableWriteModeStep

    User ->> RestoreBackup: Start Upgrade
    RestoreBackup ->> GMSDisableWriteModeStep: Disable Write Mode
    GMSDisableWriteModeStep -->> RestoreBackup: Write Mode Disabled
    RestoreBackup ->> ClearSystemMetadataServiceStep: Clear System Metadata
    ClearSystemMetadataServiceStep -->> RestoreBackup: System Metadata Cleared
    RestoreBackup ->> GMSEnableWriteModeStep: Enable Write Mode
    GMSEnableWriteModeStep -->> RestoreBackup: Write Mode Enabled
    RestoreBackup -->> User: Upgrade Complete

Loading

Poem

In the world of code so bright and vast,
A rabbit hopped, upgrades amassed.
With metadata cleared and steps refined,
System wrote its tales, so aligned.
Services dance in sync, anew,
Hail the changes, fresh as dew.
🌟🐇✨

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3056cd8 and b8d314f.

Files selected for processing (9)

• datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/common/steps/ClearSystemMetadataServiceStep.java (1 hunks)
• datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/common/steps/GMSDisableWriteModeStep.java (2 hunks)
• datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/common/steps/GMSEnableWriteModeStep.java (2 hunks)
• datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/RestoreBackupConfig.java (2 hunks)
• datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/RestoreIndicesConfig.java (2 hunks)
• datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/restorebackup/RestoreBackup.java (3 hunks)
• datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/restoreindices/RestoreIndices.java (3 hunks)
• metadata-io/src/main/java/com/linkedin/metadata/graph/elastic/ElasticSearchGraphService.java (2 hunks)
• metadata-io/src/main/java/com/linkedin/metadata/systemmetadata/ElasticSearchSystemMetadataService.java (2 hunks)

Additional comments not posted (27)

datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/common/steps/GMSDisableWriteModeStep.java (1)

32-32: Method call correctness.

The method call systemEntityClient.setWritable(context.opContext(), false) appears correct, assuming setWritable accepts these parameters.

datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/common/steps/GMSEnableWriteModeStep.java (2)

31-31: Method call correctness.

The method call systemEntityClient.setWritable(context.opContext(), true) appears correct, assuming setWritable accepts these parameters.

---

15-15: Field renaming consistency.

Ensure that the renaming from entityClient to systemEntityClient is consistent across the codebase.

datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/common/steps/ClearSystemMetadataServiceStep.java (5)

16-20: Constructor initialization correctness.

The constructor correctly initializes the _systemMetadataService and _alwaysRun fields.

---

22-25: Method implementation correctness.

The id method correctly returns the string identifier "ClearSystemMetadataServiceStep".

---

28-37: Method implementation correctness.

The skip method correctly determines if the step should be skipped based on the context and _alwaysRun flag.

---

39-42: Method implementation correctness.

The retryCount method correctly returns the number of retries as 1.

---

45-54: Method implementation correctness.

The executable method correctly returns a function that clears the system metadata service and handles exceptions properly.

datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/RestoreIndicesConfig.java (2)

24-43: Method implementation correctness.

The createInstance method correctly initializes and returns a RestoreIndices instance with the new SystemMetadataService dependency.

---

51-51: Method implementation correctness.

The createNotImplInstance method correctly returns a RestoreIndices instance with null dependencies and logs a warning for unsupported configurations.

datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/RestoreBackupConfig.java (5)

9-9: LGTM!

The import of SystemMetadataService is correct and necessary for the changes.

---

30-32: LGTM!

The addition of systemMetadataService as a dependency is correct and consistent with the PR objectives.

---

40-46: LGTM!

The correct beans are being retrieved from the application context, including the newly added SystemMetadataService.

---

50-56: LGTM!

The RestoreBackup instance is being created with the correct parameters, including the newly added systemMetadataService.

---

64-64: LGTM!

The RestoreBackup instance is being created with null parameters for unsupported configurations, which is appropriate.

datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/restoreindices/RestoreIndices.java (5)

13-13: LGTM!

The import of SystemMetadataService is correct and necessary for the changes.

---

37-37: LGTM!

The SystemMetadataService has been correctly added as a parameter in the constructor.

---

41-43: LGTM!

The buildSteps method is correctly called with the new systemMetadataService parameter.

---

62-62: LGTM!

The SystemMetadataService has been correctly added as a parameter in the buildSteps method.

---

66-66: LGTM!

The ClearSystemMetadataServiceStep is correctly added to the steps with the new systemMetadataService parameter.

datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/restorebackup/RestoreBackup.java (5)

17-17: LGTM!

The import of SystemMetadataService is correct and necessary for the changes.

---

31-34: LGTM!

The SystemMetadataService has been correctly added as a parameter in the constructor.

---

38-44: LGTM!

The buildSteps method is correctly called with the new systemMetadataService parameter.

---

64-67: LGTM!

The SystemMetadataService has been correctly added as a parameter in the buildSteps method.

---

70-70: LGTM!

The ClearSystemMetadataServiceStep is correctly added to the steps with the new systemMetadataService parameter.

metadata-io/src/main/java/com/linkedin/metadata/systemmetadata/ElasticSearchSystemMetadataService.java (1)

Line range hint 111-111: LGTM!

The removal of the @VisibleForTesting annotation is appropriate since the method is used beyond testing.

metadata-io/src/main/java/com/linkedin/metadata/graph/elastic/ElasticSearchGraphService.java (1)

Line range hint 147-150: LGTM! But verify the method usage in the codebase.

The code changes are approved.

However, ensure that the clear method is used appropriately in the codebase now that it is intended for production use.

Verification successful

---

Verification Successful: Appropriate Usage of clear Method in Production Code

The clear method in ElasticSearchGraphService is invoked in several production files, confirming its intended use in production code.

• Files with clear method invocation:
  • metadata-io/src/main/java/com/linkedin/metadata/graph/elastic/ElasticSearchGraphService.java
  • metadata-service/services/src/main/java/com/linkedin/metadata/search/EntitySearchService.java
  • metadata-service/services/src/main/java/com/linkedin/metadata/systemmetadata/SystemMetadataService.java
  • metadata-io/src/main/java/com/linkedin/metadata/search/elasticsearch/update/ESWriteDAO.java
  • datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/common/steps/ClearGraphServiceStep.java
  • And others.

The method's usage appears consistent with its intended functionality.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the usage of the `clear` method in the codebase.

# Test: Search for the method usage. Expect: Appropriate usage in production code.
rg --type java $'clear()'

Length of output: 13973

---

---

Script:

#!/bin/bash
# Description: Verify the specific usage of the clear method in ElasticSearchGraphService and related files.

# Test: Search for the clear method in ElasticSearchGraphService.java and its related test files.
rg --type java --context 5 'clear()' metadata-io/src/main/java/com/linkedin/metadata/graph/elastic/ElasticSearchGraphService.java

Length of output: 412

---

---

Script:

#!/bin/bash
# Description: Verify the invocation of the clear method in production code.

# Test: Search for the invocation of the clear method in production files.
rg --type java --files-with-matches 'clear()' | grep -v 'test'

Length of output: 2839

[coderabbitai]

Tip

Codebase Verification

Field renaming consistency incomplete.

The renaming from entityClient to systemEntityClient is not consistently applied across the codebase. There are still multiple instances of entityClient present.

• Ensure all occurrences of entityClient are renamed to systemEntityClient in the entire codebase.

Analysis chain

Field renaming consistency.

Ensure that the renaming from entityClient to systemEntityClient is consistent across the codebase.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify that all instances of `entityClient` have been renamed to `systemEntityClient`.

# Test: Search for the old field name. Expect: No occurrences of `entityClient`.
rg --type java $'entityClient'

Length of output: 214496