Skip to content

HDDS-9432. Perform individual delete key instead of delete range function#5454

Merged
swamirishi merged 10 commits intoapache:masterfrom
swamirishi:HDDS-9432
Oct 19, 2023
Merged

HDDS-9432. Perform individual delete key instead of delete range function#5454
swamirishi merged 10 commits intoapache:masterfrom
swamirishi:HDDS-9432

Conversation

@swamirishi
Copy link
Contributor

@swamirishi swamirishi commented Oct 17, 2023

What changes were proposed in this pull request?

Currently rocksDb sst file becomes corrupt in case of delete range function while creating snapshot. Speculating the corruption could be because of delete range function. When a snapshot for a bucket is created, all keys corresponding to the bucket in the deletedTable & deletedDirTable are removed from active object store after creating the rocksdb checkpoint.

What is the link to the Apache JIRA:

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

How was this patch tested?

Existing unit tests. Issue is not reproducible

Copy link
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 for the patch @swamirishi

}

// Clean up deletedDirectoryTable
deleteRangeInclusive(omMetadataManager.getDeletedDirTable(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this method. It is not used anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let us keep this method. We might have to dig further as to why deleteRange is failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment to not remove it. Someone might just go over and remove it since it is not used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@swamirishi swamirishi marked this pull request as draft October 18, 2023 18:48
@swamirishi swamirishi marked this pull request as ready for review October 18, 2023 18:49
Copy link
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.

Overall looks good to me.

Left couple of nit comments.

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.

thanks @swamirishi for this PR. Overall the changes look good to me.
Lets test/verify if this avoids the rocksDB crash that we are seeing in deletRange. If not, we are probably better off with the use of deleteRange API.

@swamirishi
Copy link
Contributor Author

thanks @swamirishi for this PR. Overall the changes look good to me. Lets test/verify if this avoids the rocksDB crash that we are seeing in deletRange. If not, we are probably better off with the use of deleteRange API.

Sure

@swamirishi
Copy link
Contributor Author

Thank you @hemantk-12 @prashantpogde for reviewing the patch.

@swamirishi swamirishi merged commit 33140cc into apache:master Oct 19, 2023
ibrusentsev pushed a commit to ibrusentsev/ozone that referenced this pull request Nov 14, 2023
…tion (apache#5454)

* HDDS-9432. Perform individual delete key instead of delete range function

* HDDS-9432. Fix issue

* HDDS-9432. Fix checkstyle issue

* HDDS-9432. Address review comments

* HDDS-9432. Add test cases

* HDDS-9432. Fix test case

* HDDS-9432. Address review comment

* HDDS-9432. Address review comment

* HDDS-9432. Fix checkstyle

* HDDS-9432. Fix javadocs
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Feb 1, 2024
…e range function (apache#5454)

(cherry picked from commit 33140cc)
Change-Id: I9e3f209bb0b8f5cbede4ee83012b600683a15dde
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants