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.

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

Replace deprecated RocksDB#addFile with RocksDB#ingestExternalFile #2291

Closed

Conversation

adamretter
Copy link
Collaborator

Previously the Java implementation of RocksDB#addFile was both incomplete and not inline with the C++ API.

Rather than fix it, as I see that rocksdb::DB::AddFile is now deprecated in favour of rocksdb::DB::IngestExternalFile, I have removed the old broken implementation and implemented RocksDB#ingestExternalFile.

Closes #2261

@facebook-github-bot
Copy link
Contributor

@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@sagar0
Copy link
Contributor

sagar0 commented May 15, 2017

I am not sure if there are any users of addFileWithFilePath and addFileWithFileInfo Java APIs, but, if there are such users their code will be broken with this change.
However, I do agree that we should move over to ingestExternalFile if addFile* is incomplete to start with.

@sagar0
Copy link
Contributor

sagar0 commented May 15, 2017

@adamretter Seems like the Java builds in Travis are failing in this as well as other PRs with:
clang: error: no such file or directory: 'util/build_version.cc', though not related to this PR.

I will wait for the appveyor builds to start passing again before merging this.

@adamretter
Copy link
Collaborator Author

@sagar0 Yeah, I looked into Travis and saw the build failures for the file util/build_version.cc in RocksJava. Unfortunately the Makefile and the Travis config have become complicated to a state where I can't figure out why this fails for RocksJava and not for Rocks itself.

It seems that util/build_version.cc is not generated by the Makefile for when make is called with the targets clean jclean rocksdbjava but I can't see why?!?

@mikhail-antonov
Copy link
Contributor

Looks good to me.

@sagar0
Copy link
Contributor

sagar0 commented May 16, 2017

@adamretter Could you please rebase and push again?
The corresponding appveyor build failed due to an unrelated reason (blob_db_test): https://ci.appveyor.com/project/Facebook/rocksdb/build/1.0.4382 , but now that the windows builds are fixed, I want to make sure that this PR passes the appveyor build before merging.

@facebook-github-bot
Copy link
Contributor

@adamretter updated the pull request - view changes - changes since last import

@adamretter
Copy link
Collaborator Author

@sagar0 Yup, done :-)

@facebook-github-bot
Copy link
Contributor

@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@sagar0
Copy link
Contributor

sagar0 commented May 17, 2017

Sorry for asking again, but possible to do another rebase? I just fixed the appveyor/windows build break (in #2309), and we can't have it broken again.

@facebook-github-bot
Copy link
Contributor

@adamretter updated the pull request - view changes - changes since last import

@adamretter
Copy link
Collaborator Author

@sagar0 Yup. Done

@sagar0
Copy link
Contributor

sagar0 commented May 18, 2017

The java builds on travis failed. Can you take a look?

error: no such file or directory: 'java/rocksjni/ingest_external_file_options.cc'

@facebook-github-bot
Copy link
Contributor

@adamretter updated the pull request - view changes - changes since last import

@adamretter
Copy link
Collaborator Author

@sagar0 Oops! Okay I have added that now

@facebook-github-bot
Copy link
Contributor

@adamretter updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@adamretter adamretter deleted the java-ingest-external-file branch June 5, 2017 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ingesting sst files with addFile error
4 participants