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

rocksdb: sync with upstream #14456

Merged
merged 2 commits into from Apr 18, 2017

Conversation

Projects
None yet
4 participants
@tchaikov
Contributor

tchaikov commented Apr 11, 2017

to pickup the change removing "add_definitions(-DROCKSDB_JEMALLOC)" in
rocksdb's CMakeListst.txt, so FreeBSD can built without the jemalloc's
header files.

Signed-off-by: Kefu Chai kchai@redhat.com

@tchaikov tchaikov added the build/ops label Apr 11, 2017

@tchaikov tchaikov requested review from smithfarm and wjwithagen Apr 11, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 11, 2017

@tchaikov I guess it just needs a rados run or something? I'd like to test my #14415 at the same time if possible.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Apr 11, 2017

@tchaikov
Before sticking my foot in this beartrap, it would be nice to be certain that it build under the std Ceph environment.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 11, 2017

@smithfarm do you want me to pull your change into my test branch or you will do it?

@wjwithagen sure, it's needs-qa once i get an approval from any of our reviewers.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 11, 2017

@tchaikov I would be grateful if you pulled my change into your test branch!

@wjwithagen
[ 88%] Building CXX object src/test/objectstore/CMakeFiles/unittest_rocksdb_option.dir/TestRocksdbOptionParse.cc.o
/usr/srcs/Ceph/work.patch/ceph/src/test/objectstore/TestRocksdbOptionParse.cc:41:24: error: no member named
      'disableDataSync' in 'rocksdb::Options'
  ASSERT_FALSE(options.disableDataSync);
               ~~~~~~~ ^

Likely from the "upgrade"
And it looks like this option has been removed...

@tchaikov tchaikov changed the title from rocksdb: sync with upstream to [DNM] rocksdb: sync with upstream Apr 11, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Apr 11, 2017

@tchaikov
Removing that one option line, make the compile run to completion.
So that would be a (first) fix. Do you want to do that is this PR?
Maybe a separate commit?

tchaikov added some commits Apr 12, 2017

tests: unittest_rocksdb_option: remove deprecated option
* disable_data_sync was removed from rocksdb, see
  https://github.com/facebook/rocksdb/blob/02799ad77a16332ee5bfb570f62ab6162a788a5a/HISTORY.md#530-03082017.
  so remove it.
* and check options.compression, which was commented out.

Signed-off-by: Kefu Chai <kchai@redhat.com>
rocksdb: sync with upstream
to pickup the change removing "add_definitions(-DROCKSDB_JEMALLOC)" in
rocksdb's CMakeListst.txt, so FreeBSD can built without the jemalloc's
header files.

Signed-off-by: Kefu Chai <kchai@redhat.com>

@tchaikov tchaikov changed the title from [DNM] rocksdb: sync with upstream to rocksdb: sync with upstream Apr 12, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 12, 2017

@wjwithagen thanks for root causing the failure. i added it in another commit in this PR, could you take another look?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 12, 2017

@smithfarm will do.

@tchaikov tchaikov requested a review from xiaoxichen Apr 12, 2017

@xiaoxichen

The removal of disableDataSync looks good to me

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Apr 12, 2017

@tchaikov
LGTM
FreeBSD compiles this just fine.

@tchaikov tchaikov added the needs-qa label Apr 12, 2017

@tchaikov tchaikov self-assigned this Apr 12, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 14, 2017

retest this please.

@@ -38,8 +37,7 @@ TEST(RocksDBOption, simple) {
ASSERT_EQ(104857600u, options.max_bytes_for_level_base);
ASSERT_EQ(10485760u, options.target_file_size_base);
ASSERT_EQ(3, options.num_levels);
ASSERT_FALSE(options.disableDataSync);
// ASSERT_EQ("none", options.compression);
ASSERT_EQ(rocksdb::kNoCompression, options.compression);

This comment has been minimized.

@smithfarm

smithfarm Apr 14, 2017

Contributor

@tchaikov I guess rocksdb::kNoCompression is some kind of constant defined in rocksdb/db.h

Anyway, LGTM

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 14, 2017

This PR does change the submodule, so the "Unmodified Submodules" test is not expected to pass.

@tchaikov tchaikov merged commit c1ba579 into ceph:master Apr 18, 2017

2 of 3 checks passed

Unmodifed Submodules Approval needed: modified submodules found
Details
Signed-off-by all commits in this PR are signed
Details
default Build finished.
Details

@tchaikov tchaikov deleted the tchaikov:wip-rocksdb-freebsd-build branch Apr 18, 2017

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