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

Fix broken snappy support in leveldb #276

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

benjoe87
Copy link

@benjoe87 benjoe87 commented Jul 7, 2023

Without exporting the compiling flags the build_detect_platform is not able to detect the snappy support and does not set the SNAPPY C macro.

Fix issue #275

Without exporting the compiling flags the build_detect_platform is not able to detect the snappy support and does not set the SNAPPY C macro.
@systream
Copy link

systream commented Aug 9, 2023

@martinsumner how could we proceed with this.
We still using leveldb. :/

@martinsumner
Copy link
Contributor

I wanted to have a test to make sure if we do fix this issue, we're not going to regress the fix.

This updated riak_test upgrade test detects both the problem of compression not working, as well as the previously undetected issue of snappy updates being uncompatable.

However, when I test this on a non-update (e.g. previous and current both develop-3.0) but with the eleveldb branch switched to benjoe87:fix-snappy-support - the test still failed. The size of the backend store didn't reduce when comparing snappy with compression disabled:

================ verify_basic_upgrade failure stack trace =====================
{{assert,[{module,verify_basic_upgrade},
          {line,115},
          {expression,"NativeSz < floor ( ( PlainTextSz * 0.8 ) )"},
          {expected,true},
          {value,false}]},
 [{verify_basic_upgrade,'-confirm/0-fun-12-',2,
                        [{file,"/Users/martinsumner/riak_test/tests/verify_basic_upgrade.erl"},
                         {line,115}]},
  {verify_basic_upgrade,confirm,0,
                        [{file,"/Users/martinsumner/riak_test/tests/verify_basic_upgrade.erl"},
                         {line,115}]},
  {riak_test_runner,return_to_exit,3,
                    [{file,"/Users/martinsumner/riak_test/src/riak_test_runner.erl"},
                     {line,159}]}]}
===============================================================================

So I can't seem to get this PR to resolve the problem. Is there other evidence this works?

@martinsumner
Copy link
Contributor

@tburghart is there a fix for this in your version?

martinsumner added a commit to OpenRiak/eleveldb that referenced this pull request Aug 29, 2023
@martinsumner
Copy link
Contributor

Previously when I tested this fix, using the updated verify_basic_upgrade - it FAILED. But this was testing on my OSX laptop.

I tried this fix today on Ubuntu - and it worked.

  • setting current 3.0.16, previous 3.0.16 -> verify_basic_upgrade FAIL (as size of leveldb store not reduced by enabling compression).
  • setting current 3.0.16 + ELEVELDB PR276, previous 3.0.16 + ELEVELDB PR276 -> verify_basic_upgrade PASS (compression now enabled)
  • setting current 3.0.16 + ELEVELDB PR276, previous 3.0.11 -> verify_basic_upgrade PASS (1.1.9 and 1.04 are in fact backwards/forwards compatible)

This would appear to confirm this PR is good on Ubuntu. The original issue is caused by snappy not running and not due to incompatibility between 1.0.4 and 1.1.9.

As OSX is not supported for running production Riak clusters, I think the PR should be accepted so that there can be a 3.0.17 release that will allow multi_backend users (or those who have over-ridden the LZ4 default) to safely upgrade from 3.0.11.

@nsaadouni - is this something you can look at?

@martinsumner
Copy link
Contributor

The OSX issues can be resolved by setting "std=c++11" in the CFLAGS and COMMON_FLAGS in build_detect_platform:

OpenRiak/leveldb@458bfb4

Also in the rebar.config.script so that it is used when compiling eleveldb:

OpenRiak@50dd137#diff-a07a3d3b357a34c4acb80b2e178e9a19e6c2ed9d91cb0aa8d53cadabb1ae8bd0

@tburghart
Copy link
Contributor

Sorry for being late to the discussion.
The way we've "fixed" it in our version is to completely rewrite eleveldb/c_src/Makefile to:

  • clone the leveldb, leveldb_ee, and snappy repos directly by version (no submodules)
  • run (and clean up after) snappy's cmake
  • build the leveldb archive and tools using a tuned-up version of leveldb/build_detect_platform

This has given us not only a clean build across platforms, but also a tree that recognizes it's already been built so it doesn't rebuild 8 times when making devrels 🤨

@bet365-bspencer bet365-bspencer merged commit c7c71c0 into basho:develop-3.0 Nov 23, 2023
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.

5 participants