pass tests for Suor/django-cacheops repo

romange commented 2 years ago

git clone https://github.com/Suor/django-cacheops
cd django-cacheops/
pip install -r ./requirements-test.txt
./run_tests.py 2> 1.err

The first failure is due to lack of support of cjson https://github.com/mpx/lua-cjson. Redis just copied the files needed to build cjson into deps/lua/src/ together with other libraries like msgpack, struct and bit.

romange commented 2 years ago

dranikpg commented 2 years ago

The solution I see is:

Getting the lua-cjson files from github.com/mpx/lua-cjson
- We only need a subset of its files. add_third_party currently uses ExternalProject_Add. I'm not sure whether its better to extend add_third_party with some more clumsy options for extracting files from different sources or download cjson with a separate command and just cp the required files on the CONFIGURE step. The latter seems to be easier and requires less tricky code.
Patching the Makefile. There are only minor differences (redis did it)
Auto including the libraries:
- enable flags: https://github.com/redis/redis/blob/d96f47cf06b1cc24b82109e0e87ac5428517525a/deps/lua/src/lua_cjson.c#L1408
- register them: https://github.com/redis/redis/blob/8aad2ac35201fd6feadb6f59e7cc0c2afb19129b/src/script_lua.c#L1179

As a side note, redis guards global overrides in src/script_lua. This should be probably added as a TODO.

romange commented 2 years ago

Given that this project is not under active development, you also have an option to just copy the files you need under src/redis/ dir (not sure if it simplifies your life or not). Why do we need just a subset of files? Can we just download and build the project as it is (with patching if needed) ?

dranikpg commented 2 years ago

you also have an option to just copy the files

Yes, this would be even easier if this is ok. I was just skeptical of keeping files around locally when a download seems "cleaner".

Why do we need just a subset of files?

We need to make sure we don't accidentally overwrite the makefile when merging

Can we just download and build the project as it is

We can. I suppose redis inlined builds because its just easier to inject a few source files rather than building separate projects and storing all of their files.

All that cjson depends on is a lua include, which we would have after download.

Are we going to add just cjson, or all of them? Not all libraries are hosted on github: http://bitop.luajit.org/download.html gives you a zip - but I guess this could also be unpacked with cmake . If we store them locally, instead of keeping the full projects, we can just have required source files. And inject them on main lua build. The redis way in short.

The other way would be downloading the full projects and building them. I'm not sure its worth keeping full projects locally and building them separately, because joining the lua build requires just a few patched lines.

romange commented 2 years ago

I do not mind adding 3rd party lua modules under src/redis/lua - the main metric I look at is the maintenance cost. I assume that in this case is maintenance is negligible because most libraries are probably not under active development.

I prefer that we won't add them into lua build, i.e. build them separately with lua library being their dependency. It should resemble how they are usually built.
The goal is to add all lua modules that redis oss supports and not only cjson. btw, if you find it easier to start with another lib - do that.

Btw, if you look at DF lua code you can see that we already load some modules that lua provides natively - https://github.com/dragonflydb/dragonfly/blob/main/src/core/interpreter.cc#L200

dranikpg commented 2 years ago

I do not mind adding 3rd party lua modules under src/redis/lua

build them separately

Got it. So I'll add each project with its native make file and trigger its build from cmake after downloading lua?

romange commented 2 years ago

you also have an option to just copy the files

Yes, this would be even easier if this is ok. I was just skeptical of keeping files around locally when a download seems "cleaner".

Why do we need just a subset of files?

We need to make sure we don't accidentally overwrite the makefile when merging

Can we just download and build the project as it is

We can. I suppose redis inlined builds because its just easier to inject a few source files rather than building separate projects and storing all of their files.

All that cjson depends on is a lua include, which we would have after download.

Are we going to add just cjson, or all of them? Not all libraries are hosted on github: http://bitop.luajit.org/download.html gives you a zip - but I guess this could also be unpacked with cmake.

We support it natively, see https://github.com/romange/helio/blob/master/cmake/third_party.cmake#L269 for example.

romange commented 2 years ago

I do not mind adding 3rd party lua modules under src/redis/lua

build them separately

Got it. So I'll add each project with its native make file and trigger its build from cmake after downloading lua?

No, if you just copy .h,.c files into src/redis/lua then also introduce our native CMakeLists.txt which creates a library. you can easily control compiler options/defines in cmake if needed. I can help you, just ping me on discord...

romange commented 2 years ago

I took a look at bitops lib.

Unfortunately, http://bitop.luajit.org/download.html version does not work with lua >= 5.3.

I searched for answers in github and found this patch https://github.com/LuaJIT/LuaJIT/issues/384 that should fix the issue. For some reason, it did not land into the official repo, but I saw folks actually use it. Therefore, I suggest that you take the version with this patch, for example, https://github.com/cuu/gameserver/blob/32c25c2ed01b303296416963315ca3c36518e0ff/LuaBitOp-1.0.3/bit.c I verified that it only contains the necessary changes and it compiles with lua 5.4 that we use.

Hope it helps.

romange commented 1 year ago

@dranikpg Can we add a test now that covers django-cacheops ? I liked your idea to whitelist scripts by sha to improve UX (even if it's a short-term, high-maintenance solution).

dranikpg commented 1 year ago

We can, with global and non-atomic modes (possibly)

I guess this is not the right issue to discuss such changes, but we need to keep two internal infos about the script for sure:

whether its deterministic
whether its atomic

Based on this we can choose the most fitting execution mode internally, without the user needing to learn about DFs scheduling.

How can it be controlled?

With a config command that accepts a sha (but we need to memorize settings even for eval if the script it not stored)
With script pragmas, which also seems like a good idea:
```
-- df:not-atomic
-- df:undeclared
```

redis.call('GET', 'anykey')

dranikpg commented 1 year ago

With https://github.com/dragonflydb/dragonfly/pull/871, cacheops tests will finally pass. Even in non atomic mode 🤨 (I ran them multiple times). I think we can close this issue then and continue our discussion about script settings elsewhere

dranikpg commented 1 year ago

Now passing

dragonflydb / dragonfly

pass tests for Suor/django-cacheops repo #184