Skip to content

cmake: require RocksDB 5.14 or higher#29930

Merged
tchaikov merged 1 commit intoceph:masterfrom
IlsooByun:rocksdb_ver
Oct 30, 2019
Merged

cmake: require RocksDB 5.14 or higher#29930
tchaikov merged 1 commit intoceph:masterfrom
IlsooByun:rocksdb_ver

Conversation

@IlsooByun
Copy link
Contributor

@IlsooByun IlsooByun commented Aug 27, 2019

Signed-off-by: Ilsoo Byun ilsoobyun@linecorp.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs

@IlsooByun IlsooByun changed the title It is compatible with 5.10 or higher. It is compatible with RocksDB 5.10 or higher. Aug 27, 2019
@IlsooByun IlsooByun changed the title It is compatible with RocksDB 5.10 or higher. build/ops: It is compatible with RocksDB 5.10 or higher. Aug 27, 2019
@stale
Copy link

stale bot commented Oct 26, 2019

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Oct 26, 2019
@stale stale bot removed the stale label Oct 27, 2019
@tchaikov
Copy link
Contributor

tchaikov commented Oct 27, 2019

@IlsooByun why do you think we need rocksdb 5.10? i am not against this change, just want to understand the reason why we depend on this version and up.

@IlsooByun
Copy link
Contributor Author

IlsooByun commented Oct 28, 2019

@IlsooByun why do you think we need rocksdb 5.10? i am not against this change, just want to understand the reason why we depend on this version and up.

This is because when using an external rockdb library, build failed before versions 5.10 because the definition of the Env class is different. I tested it by increasing version up, and succeeded to build in 5.10.

@tchaikov
Copy link
Contributor

tchaikov commented Oct 28, 2019

@IlsooByun in that case, could you include the failure or the error message in the commit message? and revise the commit message to something like

cmake: require RocksDB 5.10 or higher

?

@tchaikov
Copy link
Contributor

tchaikov commented Oct 28, 2019

http://pulpito.ceph.com/kchai-2019-10-28_03:11:42-rados-wip-kefu-testing-2019-10-28-0018-distro-basic-smithi/

@tchaikov tchaikov self-assigned this Oct 28, 2019
@IlsooByun
cmake: require RocksDB 5.14 or higher
a83a25c 
error: ‘class rocksdb::Env’ has no member named ‘SetAllowNonOwnerAccess’
       opt.env->SetAllowNonOwnerAccess(false);
                    ^~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Ilsoo Byun <ilsoobyun@linecorp.com>
@IlsooByun
Copy link
Contributor Author

IlsooByun commented Oct 29, 2019

@tchaikov I tested it again and found there is additional change.
In the current master version, RocksDBStore class uses Env::SetAllowNonOwnerAccess method.
But this method is added in 5.14. So I changed the required version from 5.8 to 5.14.

@tchaikov tchaikov changed the title build/ops: It is compatible with RocksDB 5.10 or higher. cmake: require RocksDB 5.14 or higher Oct 30, 2019
@tchaikov
Copy link
Contributor

tchaikov commented Oct 30, 2019

for posterity, the new API was introduced by facebook/rocksdb@a0102aa

@tchaikov tchaikov merged commit 2210425 into ceph:master Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tchaikov tchaikov tchaikov approved these changes

Assignees

@tchaikov tchaikov

Labels

build/ops needs-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@IlsooByun @tchaikov @batrick