provide predis native configuration option #88

scones · 2018-06-11T15:20:33Z

the current wrapper class does not allow for a deviation from "one redis server" configuration variant of predis.

As Resque in itself is a technology one usually uses in more complex setups, the need for a more complex redis-setup arises.
The provided predis version does have all these options, but the wrapper is too limited.

This patch allows for direct configuration of predis via the predis section in the config.yml
it effectively overrides anything configured in the redis section, so one has to chose.

I provided a sentinel example for that.

In addition i added the following things for a brighter future:

Dockerfile file for testing
docker-compose.yml file for testing
a basic editorconfig
2 tests (Redis with legacy initialization, Redis with override initialization)
a .gitattributes file

if this gets merged i also have the tools to work on the handler thing again (#85 )

best regards,
scones

scones · 2018-06-11T15:40:59Z

upon rereading the issues:
this solves #63

xelan · 2018-06-13T09:40:07Z

Thank you very much for this nice PR, @scones! I'll have a more detailed look in a few days. In general, the concept to leverage the full potential of the underlying predis library sounds great. Although you also still support the legacy configuration, I'd think this should land in the 3.0 release. What is your opinion on that?

Unfortunately, I didn't get any response from @mjphaynes yet concerning issue #78. However, we could postpone that decision to 4.0 and continue now with this PR, PR #82 and PR #85 for php-resque 3.0.

xelan · 2018-06-13T09:42:27Z

By the way, could you please also add a short description in the docs? Thank you very much 😄

scones · 2018-06-13T11:43:21Z

changing the major version would indicate a major change in interfaces or a complete rewrite.
Neither is done with this request. I wrote it as compatible with all existing installations on purpose.

You can merge it as soon as you find it good. ;)

also: what to add to the docs? i edited the config.yml with the dummy configs. How to configure the predis client is not in the scope of the docs from php-resque, but the job of predis.

best regards

xelan · 2018-06-13T14:18:09Z

Thanks. I like the general approach and will review and (probably) merge soon 👍 . Two small things I saw for now:

The Dockerfile you add is for development and testing with PHP 5.3, right? Maybe we should add a general docker readme (docker.md?) in the docs folder on how to run php-resque in Docker with best practices (see #66).

Could you please use camelCase for variable and property names instead of snake_case? We try to comply with PSR-2 for new code (although there are still a few legacy code parts).

Thank you very much!
Best regards
Andreas

scones · 2018-06-13T15:00:36Z

The Dockerfile is for php 5.3, yes. But it is intended as testing environment.
I wouldn't know if it is suitable for productive use. Thus there is no warranty.

I will rename the variables.

scones · 2018-06-13T15:21:52Z

done

xelan · 2018-06-16T13:43:47Z

Great, thank you @scones 👍
I'll have a look on Monday.

Best regards
Andreas

xelan · 2018-06-19T15:53:04Z

I'll leave a few notes via the review system here 😄
Could you afterwards please rebase your commit messages (we normally use imperative form, first letter uppercase, no branch name, e.g. "Pass the predis config options through directly") and maybe squash them logically?

Thank you very much!

xelan · 2018-06-19T15:54:01Z

config.yml

@@ -11,7 +11,7 @@

 include: examples/autoload.php  # A file to include on each command

-
+# # The manual legacy way


I'm not sure whether manual is the right expression here. Maybe just "The legacy way, which only supports a subset of Redis options"?

xelan · 2018-06-19T15:55:26Z

docker/app_5_3/Dockerfile

+
+RUN apt-get install -yqq git-core
+
+RUN pecl install -o -f redis &&  rm -rf /tmp/pear &&  docker-php-ext-enable redis


If i remember correctly, we use phpiredis instead of the quite problematic and unmaintained PHP redis extension. See #42

maybe, but this configuration let me run the tests.
which is all this container is for: run the tests in any environment.

Ok, let's maybe add a comment at the beginning of the Dockerfile that it is for development and testing with the minimum deps, but not suitable as a template for production. What do you think?

We plan to add/link a fully-fledged Docker example with a minimal but complete worker for a real-world application later on anyway.

xelan · 2018-06-19T15:56:54Z

docker/app_5_3/Dockerfile

+RUN docker-php-ext-install pcntl
+
+
+CMD composer install -v && tail -f /dev/null


What is fail -f /dev/null supposed to do in this context?

tail -f attaches itself to a stream and listens. with /dev/null as a stream there will be no output, but the stream stays open.

in combination it keeps the CMD running without producing spam.
Thus the docker container won't shut down after it booted up, but stays active.

xelan · 2018-06-19T15:57:41Z

src/Resque/Redis.php

            );
+
+            // setup password
+            if (isset($config['password']) && !empty($config['password'])) {


empty includes a check for isset() so it is not necessary to do both.

sorry, force of habbit.
i tend to implement such stuff explicit and more readable

xelan · 2018-06-19T15:58:03Z

src/Resque/Redis.php

        }

        // create Predis client
-        $this->redis = new Predis\Client($params, $options);
+        $this->redis = $this->initializePredisClient($predisParams, $predisOptions);


Good idea to extract the intitalization!

this was necessary to make the initialization testable, as it it cumbersome to mock foreign objects.

xelan · 2018-06-19T15:58:19Z

tests/Resque/Helpers/UtilTest.php

@@ -9,7 +9,7 @@
 * file that was distributed with this source code.
 */

-namespace Resque\Tests\Heplers\Utils;
+namespace Resque\Tests\Helpers;


Thanks for fixing that 👍

… stuff on productive builds

…redis configuration

scones · 2018-06-20T11:39:00Z

everything updated.
some of your reviews got lost in the process tho.

xelan · 2018-06-27T09:29:44Z

Thank you very much 👍
I'll test it in an example application with different settings within the next days. Then it'll land in 2.2 if everything is fine.

Best regards
Andreas

xelan · 2018-07-16T08:03:27Z

Tests A: default config, virtual machine
Tests B: legacy config, virtual machine
Tests C: native config, virtual machine
Tests D: default config, docker swarm
Tests E: legacy config, docker swarm
Tests F: native config, docker swarm

scones · 2018-07-16T08:57:03Z

unless there is more text to that, i will assume these checkboxes mean: tests in progress :)

xelan · 2018-07-16T09:53:21Z

Exactly 😀 I'll update here and post an additional comment when all tests are done. What would you think about my three notes?

scones · 2018-07-17T08:59:48Z

what notes?

xelan · 2018-07-16T08:04:59Z

config.yml

+# # "options" is the second parameter to Predis\Client
+# predis:
+#   config:
+#     - "tcp://10.0.0.1:12345"


Maybe the config template should use the standard Redis port (6379) as well. Otherwise, this might confuse users.

xelan · 2018-07-16T08:06:07Z

config.yml

+#     - "tcp://10.0.0.3:12345"
+#   options:
+#     service: "some_redis_cluster_name"
+#     replication: "sentinel"


Here I would add a comment inside the template that advanced Predis features like password, database index, replication etc are supported, but optional. Otherwise, newbies might struggle as they can't find out why it doesn't work if they try to copy and adapt the example 😉

this and the note above would suggest the existence of a valid documentation of settings.
such a thing should not be done in a config file.

thus you are asking for a feature, which is unrelated to this pull request.

That's true, so I'd just link to the Predis documentation instead of duplicating it on our own.

If someone wants to use advanced features they can do so, but the defaults in the config template should IMHO match the ones in the legacy configuration to avoid confusion.

xelan · 2018-07-16T08:09:15Z

docker/app_5_3/Dockerfile

+
+RUN apt-get install -yqq git-core
+
+RUN pecl install -o -f redis &&  rm -rf /tmp/pear &&  docker-php-ext-enable redis


Ok, let's maybe add a comment at the beginning of the Dockerfile that it is for development and testing with the minimum deps, but not suitable as a template for production. What do you think?

We plan to add/link a fully-fledged Docker example with a minimal but complete worker for a real-world application later on anyway.

xelan · 2018-07-17T09:01:03Z

Ah sorry, just saw that GitHub doesn't show my review comments to others until I explicitely publish them 😀

scones · 2018-07-17T10:30:50Z

the docker image is not referenced by any of the documentation.
it actually is viable for productive use, just not good at it (outdated php, no actual task or service provided)

thus it just exists there, without users knowing why. I can only assume, that users who seek an (undocumented) Dockerfile, will know enough about docker to understand what it's doing.
Worst case scenario: they write a bug report for an (yet) unsupported feature.

xelan added the Enhancement label Jun 13, 2018

xelan added Waiting for feedback Work in progress labels Jun 13, 2018

xelan added this to the 2.2 milestone Jun 19, 2018

xelan reviewed Jun 20, 2018

View reviewed changes

scones force-pushed the master branch 3 times, most recently from ffe59f3 to bb65341 Compare June 20, 2018 11:37

scones added 7 commits June 20, 2018 13:38

Added docker image for testing and gitattributes to ignore the docker…

261a2a3

… stuff on productive builds

Typo by original author

e3922a3

General standards

f921003

Externalize foreign object construction fro testability

16647c9

Style change

452060e

Cleaner namespacing

1f71e92

Rewrote Redis Initialization and made it testable to support direct p…

07b534e

…redis configuration

scones force-pushed the master branch from bb65341 to 07b534e Compare June 20, 2018 11:38

xelan removed Waiting for feedback Work in progress labels Jun 27, 2018

xelan reviewed Jul 17, 2018

View reviewed changes

xelan merged commit 29f331c into mjphaynes:master Oct 1, 2018

xelan mentioned this pull request Oct 1, 2018

Can not select database index when connect redis. #63

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provide predis native configuration option #88

provide predis native configuration option #88

scones commented Jun 11, 2018

scones commented Jun 11, 2018

xelan commented Jun 13, 2018 •

edited

Loading

xelan commented Jun 13, 2018

scones commented Jun 13, 2018

xelan commented Jun 13, 2018

scones commented Jun 13, 2018

scones commented Jun 13, 2018

xelan commented Jun 16, 2018

xelan commented Jun 19, 2018

xelan Jun 19, 2018

xelan Jun 19, 2018

scones Jun 20, 2018

xelan Jul 16, 2018

xelan Jun 19, 2018

scones Jun 20, 2018

xelan Jun 19, 2018

scones Jun 20, 2018

xelan Jun 19, 2018

scones Jun 20, 2018

xelan Jun 19, 2018

scones commented Jun 20, 2018

xelan commented Jun 27, 2018

xelan commented Jul 16, 2018 •

edited

Loading

scones commented Jul 16, 2018

xelan commented Jul 16, 2018

scones commented Jul 17, 2018

xelan Jul 16, 2018

xelan Jul 16, 2018

scones Jul 17, 2018

xelan Jul 18, 2018

xelan Jul 16, 2018

xelan commented Jul 17, 2018

scones commented Jul 17, 2018

		@@ -11,7 +11,7 @@

		include: examples/autoload.php # A file to include on each command


		# # The manual legacy way


		RUN apt-get install -yqq git-core

		RUN pecl install -o -f redis && rm -rf /tmp/pear && docker-php-ext-enable redis

		RUN docker-php-ext-install pcntl


		CMD composer install -v && tail -f /dev/null

provide predis native configuration option #88

provide predis native configuration option #88

Conversation

scones commented Jun 11, 2018

scones commented Jun 11, 2018

xelan commented Jun 13, 2018 • edited Loading

xelan commented Jun 13, 2018

scones commented Jun 13, 2018

xelan commented Jun 13, 2018

scones commented Jun 13, 2018

scones commented Jun 13, 2018

xelan commented Jun 16, 2018

xelan commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scones commented Jun 20, 2018

xelan commented Jun 27, 2018

xelan commented Jul 16, 2018 • edited Loading

scones commented Jul 16, 2018

xelan commented Jul 16, 2018

scones commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xelan commented Jul 17, 2018

scones commented Jul 17, 2018

xelan commented Jun 13, 2018 •

edited

Loading

xelan commented Jul 16, 2018 •

edited

Loading