Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove deprecated DB::DeleteFile API references #13322

Closed
wants to merge 4 commits into from
Closed

Remove deprecated DB::DeleteFile API references #13322

wants to merge 4 commits into from

Conversation

mszeszko-meta
Copy link
Contributor

@mszeszko-meta mszeszko-meta commented Jan 22, 2025 •
edited
Loading

Summary

Cleanup post #13284.

Test plan

  1. We did not find any evidence of breakage in internal pre-release integration pipeline runs after renaming the deprecated API in 9.10.
  2. To the extent possible, we manually validated partner use cases of file deletion and confirmed deprecated API is no longer in use.

@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mszeszko-meta mszeszko-meta force-pushed the remove_deprecated_delete_db_api_references branch from 6357c9d to b498b4b Compare January 22, 2025 05:57
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta force-pushed the remove_deprecated_delete_db_api_references branch from b498b4b to c753162 Compare January 22, 2025 06:00
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

jaykorean
jaykorean reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@jaykorean jaykorean left a comment

Choose a reason for hiding this comment

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

Looks like there's another Java test that uses this deprecated API.

There was 1 failure:
1) onTableFileDeleted(org.rocksdb.EventListenerTest)
org.junit.ComparisonFailure: expected:<[tru]e> but was:<[fals]e>
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at org.rocksdb.EventListenerTest.deleteTableFile(EventListenerTest.java:91)
	at org.rocksdb.EventListenerTest.onTableFileDeleted(EventListenerTest.java:105)

I will take another look when it's fixed. Please let me know. Thanks!

@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta marked this pull request as draft January 22, 2025 21:08
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta force-pushed the remove_deprecated_delete_db_api_references branch from 177ff0c to a3050e2 Compare January 22, 2025 21:12
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta force-pushed the remove_deprecated_delete_db_api_references branch from a3050e2 to f84312a Compare January 22, 2025 23:18
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta force-pushed the remove_deprecated_delete_db_api_references branch from 09c5e0b to 82609db Compare January 23, 2025 00:06
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta force-pushed the remove_deprecated_delete_db_api_references branch from 82609db to 23605fb Compare January 23, 2025 00:25
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta marked this pull request as ready for review January 23, 2025 00:25
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mszeszko-meta
Copy link
Contributor Author

mszeszko-meta commented Jan 23, 2025 •
edited
Loading

Looks like there's another Java test that uses this deprecated API.

There was 1 failure:
1) onTableFileDeleted(org.rocksdb.EventListenerTest)
org.junit.ComparisonFailure: expected:<[tru]e> but was:<[fals]e>
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at org.rocksdb.EventListenerTest.deleteTableFile(EventListenerTest.java:91)
	at org.rocksdb.EventListenerTest.onTableFileDeleted(EventListenerTest.java:105)

I will take another look when it's fixed. Please let me know. Thanks!

This one required a bit more work than originally anticipated. Test is passing and PR is ready for the review.

hx235
hx235 reviewed Jan 23, 2025
View reviewed changes
db/db_test.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta force-pushed the remove_deprecated_delete_db_api_references branch from 94b676e to 9bdb1f8 Compare January 24, 2025 00:36
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta force-pushed the remove_deprecated_delete_db_api_references branch from 17d7e80 to 9ea1079 Compare January 24, 2025 03:41
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@mszeszko-meta mszeszko-meta force-pushed the remove_deprecated_delete_db_api_references branch from 9ea1079 to fb106f8 Compare January 24, 2025 18:57
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jaykorean
jaykorean approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@jaykorean jaykorean left a comment

Choose a reason for hiding this comment

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

👍

db/db_test.cc Show resolved Hide resolved
db/db_test.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mszeszko-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mszeszko-meta merged this pull request in 591f5b1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaykorean jaykorean jaykorean approved these changes

@pdillinger pdillinger Awaiting requested review from pdillinger

@hx235 hx235 Awaiting requested review from hx235

@jowlyzhang jowlyzhang Awaiting requested review from jowlyzhang

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mszeszko-meta @facebook-github-bot @jowlyzhang @jaykorean @hx235