basho / eleveldb

Erlang LevelDB API
263 stars 176 forks source link

Fix broken snappy support in leveldb #276

Closed benjoe87 closed 9 months ago

benjoe87 commented 1 year ago

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 https://github.com/basho/eleveldb/issues/275

systream commented 1 year ago

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

martinsumner commented 1 year ago

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 commented 1 year ago

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

martinsumner commented 1 year ago

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.

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 commented 1 year ago

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

https://github.com/nhs-riak/leveldb/commit/458bfb4e41d2822b9f2e90b393ec786689249c33

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

https://github.com/nhs-riak/eleveldb/commit/50dd137fd8aa00a4af71d3c11af9528b0a2f40c3#diff-a07a3d3b357a34c4acb80b2e178e9a19e6c2ed9d91cb0aa8d53cadabb1ae8bd0

tburghart commented 11 months ago

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:

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 🤨