Switch to using predis to interact with redis

[icewind1991]

Use predis instead of the php-redis extension, predis is a pure php implementation of redis so it does not require any php extension to be installed.

Additionally predis seems to be more widely supported by 3rdparty stuff so using it ourselves makes it easier to integrate 3rdparty libraries that use redis.

Supersedes #23999

Requires owncloud-archive/3rdparty#273

cc @PVince81 @Xenopathic

[DeepDiver1975]

I'd like to see performance comparison between the native and the php implementation

[DeepDiver1975]

@cmonteroluque @karlitschek this change has impact on all our packaging efforts - the php-redis extension can be dropped if this works out.

[RobinMcCorkell]

@DeepDiver1975 https://github.com/nrk/predis/blob/v1.0/FAQ.md#speaking-about-performances

TL;DR: With the phpiredis native PHP extension to help with serializing/unserializing data, it has near-identical performance to phpredis. Without the native helper extension, is about 3x slower (in terms of access latency to a remote Redis server). However, the scale at which the test was performed far exceeds anything we'd ever use it for: 0.132 seconds to fetch 30000 keys.

So in summary, I'm highly in favour of this, but we should probably also package the phpiredis extension for enterprise users who need all the performance they can get.

[ghost]

nice. Also copying @jnweiger

[DeepDiver1975]

@DeepDiver1975 https://github.com/nrk/predis/blob/v1.0/FAQ.md#speaking-about-performances

I know this one - I still want to see how this performs in our usage scope

[karlitschek]

I like dropping of external dependencies. This is always great.
So we switch to this PHP implementation only or keep on using the phpredis extension only.
But we can't support both because then we have two code path and double the testing.

So we would need some real world performance tests and go with the php based solution or stick with the php extension. But not both please.

[ghost]

Fixes: #21521

[butonic]

@Xenopathic obsoletes #23204 ?

[RobinMcCorkell]

@butonic Yes, it does (just need to document how to configure a cluster)

[icewind1991]

I still want to see how this performs in our usage scope

This is the worst case redis usage I can think of within our usage with thousands locking operations in a single request.

For more "normal" cache/locking usage (regular propfind/get/put requests) I couldn't measure any significant difference between the two redis implementations since we usually any make a handful of redis calls

The redis operations are about 2-2.5 times slower against a local redis instance without using the phpiredis extension, as the predis docs say the added php overhead quickly becomes less significant when taking into account real life setups where redis is not local.

[RobinMcCorkell]

@icewind1991 Was that trace with the phpiredis extension?

[icewind1991]

No, just plain ol php

[RobinMcCorkell]

So with one of the worst possible situations, we add an extra 625ms on to the request. Nice!

[PVince81]

@karlitschek @DeepDiver1975 are the results acceptable ?

[DeepDiver1975]

@icewind1991 can you please elaborate on the test scenario you used for generating the graphs? THX

[karlitschek]

I'm ok with this once @DeepDiver1975 is.

[icewind1991]

can you please elaborate on the test scenario you used for generating the graphs? THX

running the file scanner on a folder with ~2k files

[DeepDiver1975]

running the file scanner on a folder with ~2k files

what as the difference in execution time?
Can you rerun this with a remote redis server?

THX

[DeepDiver1975]

conflicting ...

[icewind1991]

rebased

what as the difference in execution time?

As the trace says ~30%

Can you rerun this with a remote redis server?

When running against a redis server on the LAN I get about a ~7% performance decrease (again without using php extension for speedup)

(note again that this is ~7% in a worse case scenario, for regular requests there is no measurable difference)

[DeepDiver1975]

As the trace says ~30%

30% of 10 hrs or 10% of 10 minutes - this is what I looking for 😉

[icewind1991]

30% of 10 hrs or 10% of 10 minutes - this is what I looking for

The absolute time is a fairly useless metric here, it all depends on how large the folder is you're scanning

[DeepDiver1975]

What are the reasons for adding this besides the cluster support?

[icewind1991]

• no more dependencies on 3rdparty php plugins
• predis has wider support for 3rdparty php libraries that use redis (we don't want 2 difference redis implementations active)

[DeepDiver1975]

predis has wider support for 3rdparty php libraries that use redis

relevant of us currently?

we don't want 2 difference redis implementations active)

for sure not

[icewind1991]

relevant of us currently?

laravel\queue requires it for it's redis backend

[DeepDiver1975]

Okay - but there is currently no need in storing the job Queue in redis. Pure DB based jobqueue is all we need for now.

[icewind1991]

Pure DB based jobqueue is all we need for now.

using laravel's queue with the db backend pulls in laravel's db layers with it from what I can tell so that would not be optimal, but that's a separate discussion that shouldn't take place here

[butonic]

Using predis would remove some ugly php-redis dependencies for suse and redhat builds. @jnweiger may have more details. But my take is that predis is great because it works out of the box. No need to use pecl. If you really need that extra bit of performance add the drop in c bindings. Very elegant. Sorry for picking the wrong lib in the first place.

[DeepDiver1975]

Sorry for picking the wrong lib in the first place.

It's generally hard to declare something to be a wrong decision - there are always pros and cons ...

[DeepDiver1975]

using laravel's queue with the db backend pulls in laravel's db layers with it from what I can tell so that

this was my very first comment as we have been chatting about this some days back ....

[DeepDiver1975]

So currently I see no hard requirement to change the library (yes there are reasons which might let predis appear to look more elegant - but we have no hard requirement to change) and given the fact that we (as in I who has to make the decision) cannot fully judge the impact as of now and it appears to have a potential performance drawback I advise against this change.

@karlitschek @cmonteroluque

[butonic]

Packaging issues are actually the bigger problem for our customers that have to stick to rhel or suse.

[DeepDiver1975]

Packaging issues are actually the bigger problem for our customers that have to stick to rhel or suse.

From my understanding this has already been taken care of in most cases ... and if not it has to be taken care of because all installation using 8.2 and 9.0 need the extension any how.

[karlitschek]

Discussed a bit with @MTRichards and @cmonteroluque I think this is an interesting direction and something to investigate. I think the consensus is to not actively switch to this in the 9.1 timeframe. Mainly because there is: 1. no burning need for it at the moment. 2. there is always the risk to break stuff and running into new bugs when switching do a different library. 3. Already tooo much stuff to do for 9.1 Search for example.

I hope this makes sense.

[DeepDiver1975]

moving to backlog because of the reasons above

[jnweiger]

https://software.opensuse.org/download.html?project=isv%3AownCloud%3Adevel&package=php5-redis
has redis bindings for SLE, openSUSE and Ubuntu. Please be more specific about the 'bigger issue is packaging' issue -- afaik we have all major platforms covered.

What is the planned deployment method for predis? Do we include the code as 3rdparty? I see packages libphp-predis / php5-pear-nrk-Predis on many more platforms compared to php5-redis. If we include the code, we should check there is no conflicts / confusion with preinstalled packaged/predis versions.

[butonic]

Hm, then we need to reevaluate #23204 for redis cluster support, another thing this would have solved. Or in the spirit of the Manifesto for Agile Software Development:

When faced with two or more alternatives that deliver roughly the same value, take the path that makes future change easier.

Since kibana uses predis this PR will cause less trouble in the future compared with #23204

[DeepDiver1975]

Hm, then we need to reevaluate #23204 for redis cluster support,

see #23204 (comment)

[DeepDiver1975]

@icewind1991 closing this for now - THX

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.