Skip to content

HDDS-10262. Encapsulate SnapshotCache inside OmSnapshotManager#6135

Merged
hemantk-12 merged 6 commits intoapache:masterfrom
Cyrill:HDDS-10262
Feb 9, 2024
Merged

HDDS-10262. Encapsulate SnapshotCache inside OmSnapshotManager#6135
hemantk-12 merged 6 commits intoapache:masterfrom
Cyrill:HDDS-10262

Conversation

@Cyrill
Copy link
Copy Markdown
Contributor

@Cyrill Cyrill commented Jan 31, 2024 •
edited
Loading

HDDS-10262. Encapsulate SnapshotCache inside OmSnapshotManager

Changes made:
SnapshotCache is now used purely in OmSnapshotManager and not exposed outside.
Previous calls to SnapshotCache are now changed to OmSnapshotManager.
Explicit parameter "skipActiveCheck" was removed, active state check is done implicitly.
Also changed the generics in snapshot's ReferenceCounted returned value to get rid of type casts all over the codebase.
Removed unused parameter from ReferenceCounted class.

Reasoning
SnapshotCache is now hidden from the world and any further refactoring to it should be easier.

What is the link to the Apache JIRA

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

How was this patch tested?

Existing unit tests. There was no new functionality added.

@Cyrill
Copy link
Copy Markdown
Contributor Author

Cyrill commented Jan 31, 2024

@hemantk-12 please take a look at this PR

@hemantk-12 hemantk-12 requested a review from smengcl January 31, 2024 19:07
@hemantk-12 hemantk-12 added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Jan 31, 2024
hemantk-12
hemantk-12 reviewed Feb 2, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@hemantk-12 hemantk-12 left a comment

Choose a reason for hiding this comment

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

Thanks @Cyrill for the improvements.

Overall looks good to me. Left some inline comment.

@hemantk-12
Copy link
Copy Markdown
Contributor

hemantk-12 commented Feb 2, 2024

@smengcl can you please also take a look?

Cyrill added 2 commits February 5, 2024 12:24
@Cyrill
Merge remote-tracking branch 'origin/master' into HDDS-10262
575e81a 
# Conflicts:
#	hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java
@Cyrill
HDDS-10262. Fix review comments
4bcf845
@Cyrill Cyrill requested a review from hemantk-12 February 6, 2024 12:22
hemantk-12
hemantk-12 reviewed Feb 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@hemantk-12 hemantk-12 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

@smengcl and @aswinshakil, please take a look as well.

aswinshakil
aswinshakil reviewed Feb 8, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@aswinshakil aswinshakil left a comment

Choose a reason for hiding this comment

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

Thanks @Cyrill for this effort of refactoring. Most of the changes looks good to me. I have a comment inline.

...-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/ReferenceCounted.java
/**
* Parent instance whose callback will be triggered upon this RC closure.
*/
private final U parentWithCallback;
Copy link
Copy Markdown
Member

@aswinshakil aswinshakil Feb 8, 2024

Choose a reason for hiding this comment

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

I don't think we should remove this. Though we don't use it right now. There is an upcoming patch which is using this callback. Please take a look at #6024. So we don't have to remove this and add it again in #6024 .

Copy link
Copy Markdown
Contributor Author

@Cyrill Cyrill Feb 8, 2024

Choose a reason for hiding this comment

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

I'm fine with leaving the unused field, though I strongly believe the callback should not be a part of the class generics.

Copy link
Copy Markdown
Contributor Author

@Cyrill Cyrill Feb 8, 2024

Choose a reason for hiding this comment

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

Returned back the field.

@Cyrill Cyrill requested a review from aswinshakil February 8, 2024 10:02
@Cyrill
Copy link
Copy Markdown
Contributor Author

Cyrill commented Feb 9, 2024

Are we okay to merge this pr? Who should I ask to do that?

hemantk-12
hemantk-12 approved these changes Feb 9, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@hemantk-12 hemantk-12 left a comment

Choose a reason for hiding this comment

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

Thanks @Cyrill for the refactoring and clean up.

@hemantk-12 hemantk-12 merged commit 2f2234c into apache:master Feb 9, 2024
@Cyrill Cyrill deleted the HDDS-10262 branch February 11, 2024 14:36
xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024
@Cyrill @xichen01
HDDS-10262. Encapsulate SnapshotCache inside OmSnapshotManager (apach…
75c79e1 
…e#6135)

(cherry picked from commit 2f2234c)
xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024
@Cyrill @xichen01
HDDS-10262. Encapsulate SnapshotCache inside OmSnapshotManager (apach…
f0ce881 
…e#6135)

(cherry picked from commit 2f2234c)
xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024
@Cyrill @xichen01
HDDS-10262. Encapsulate SnapshotCache inside OmSnapshotManager (apach…
9b6a776 
…e#6135)

(cherry picked from commit 2f2234c)
xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 18, 2024
@Cyrill @xichen01
HDDS-10262. Encapsulate SnapshotCache inside OmSnapshotManager (apach…
1dd3005 
…e#6135)

(cherry picked from commit 2f2234c)
xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 18, 2024
@Cyrill @xichen01
HDDS-10262. Encapsulate SnapshotCache inside OmSnapshotManager (apach…
01754f7 
…e#6135)

(cherry picked from commit 2f2234c)
@xichen01 xichen01 mentioned this pull request Jul 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hemantk-12 hemantk-12 hemantk-12 approved these changes

@smengcl smengcl Awaiting requested review from smengcl

@aswinshakil aswinshakil Awaiting requested review from aswinshakil

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Cyrill @hemantk-12 @aswinshakil