Skip to content

Conversation

@smengcl
Copy link
Contributor

@smengcl smengcl commented May 3, 2023

What changes were proposed in this pull request?

Add an option to control whether Ozone filesystem snapshot feature is enabled or not on the OM side.

  1. Add this new OM config.
  2. Enable Ozone (filesystem) snapshot feature by default to avoid disruption with on-going dev work.
  3. When disabled:
    a) All snapshot related OM background tasks shall not run
    b) Snapshot requests (CreateSnapshot / DeleteSnapshot) are gracefully rejected (OMException with FEATURE_NOT_ENABLED), achieved by a new AspectJ annotation implementation.
  4. All existing and upcoming integration tests, UTs and acceptance tests related to Ozone snapshot feature would have a line to explicitly enable the snapshot feature for testing so those tests would still run and pass even when the default is switched to false.

There is a follow-up JIRA to refactor OmSnapshotManager constructor to further reduce the initialization done when snapshot feature is disabled: HDDS-8529.

TODO

  • Remove extra omSnapshotManager null checks added in this PR now that we can't really not initialize it.

What is the link to the Apache JIRA

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

How was this patch tested?

@smengcl smengcl added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label May 3, 2023
@smengcl smengcl marked this pull request as ready for review May 4, 2023 02:10
@smengcl smengcl requested a review from kerneltime May 4, 2023 06:27
smengcl added 2 commits May 4, 2023 01:54
Conflicts:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
Copy link
Contributor

@prashantpogde prashantpogde left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

LGTM. @smengcl @prashantpogde can you file a follow up jira to define the long term story around this config flag? We need to decide if moving it from on to off and back to on is supported, and add corresponding tests or docs for whatever decision is made.

@smengcl
Copy link
Contributor Author

smengcl commented May 5, 2023

LGTM. @smengcl @prashantpogde can you file a follow up jira to define the long term story around this config flag? We need to decide if moving it from on to off and back to on is supported, and add corresponding tests or docs for whatever decision is made.

Thanks @errose28 . I filed HDDS-8555 as a follow-up.

@smengcl smengcl merged commit 831f608 into apache:master May 5, 2023
@smengcl smengcl deleted the HDDS-8166-snapshot-config branch May 5, 2023 20:06
@smengcl
Copy link
Contributor Author

smengcl commented May 5, 2023

Thanks @hemantk-12 @prashantpogde @errose28 for reviewing this.

@arp7
Copy link
Contributor

arp7 commented May 8, 2023

We may not want to invest too much time in a long-term story, this flag is just for short-term use until the snapshot feature is fully ready and stable.

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.

5 participants