Named cache #114

KarelCemus · 2017-08-08T10:39:57Z

This PR redesigns configuration to fully implement named caches. Fixes #106.

This is intended for version 2.x.x without backward compatibility of the configuration. There will be warnings and migration guide to allow more advanced usage.

@pcejrowski @rval May I ask you to review this PR, especially the reference.conf file? It should be final and stable. Modules and Components will change for sure, some other binding-related classes might change too but the configuration proposal is final.

pcejrowski · 2017-08-08T15:20:46Z

Hey, sure. I'm going to review reference.conf tomorrow and I'll take a deeper look on the other files during the weekend. I'm short with time earlier.

KarelCemus · 2017-08-08T15:32:04Z

Great, thanks! No rush. And please ignore tests, they are about to be fully rewritten. And the module and the components are now also stable, I merged their update.

pelepelin · 2017-08-08T17:13:48Z

src/main/resources/reference.conf

+  #
+  # for more details see https://www.playframework.com/documentation/2.6.x/ScalaCache#Accessing-different-caches
+  #
+  instance {


I would make it instances

pelepelin · 2017-08-08T17:15:49Z

src/main/resources/reference.conf

+  instance {
+
+    # definition of the default cache,
+    # its name is defined in 'play.cache.defaultCache'


play.cache.redis.default-cache is defined below, but play.cache.defaultCache is mentioned here

KarelCemus · 2017-08-09T16:24:01Z

Thank you @checat for your feedback, it's fixed.

rval

Looks great! Some really minor feedback, but nothing that should hold up the PR IMO.

Appreciate the quick turnaround on this work; some excellent customer service! 😀

rval · 2017-08-09T22:55:59Z

src/main/resources/reference.conf

+    # definition of the default cache,
+    # its name is defined in 'play.cache.default-cache'
+    #
+    # when there should be more caches, defined them all under the 'instance' key.


Very minor point, there are some places in the comments that still reference instance instead of the updated instances.

rval · 2017-08-09T23:10:17Z

src/main/resources/reference.conf

+  #
+  # note: this is global definition, can be locally overriden for each
+  # cache instance. To do so, redefine this property
+  # under 'play.cache.redis.instance.instance-name.this-property'.


rval · 2017-08-09T23:10:30Z

src/main/resources/reference.conf

+  #
+  # note: this is global definition, can be locally overriden for each
+  # cache instance. To do so, redefine this property
+  # under 'play.cache.redis.instance.instance-name.this-property'.


KarelCemus · 2017-08-10T06:52:17Z

@rval Thanks for your feedback, I'm sorry for my sloppiness. It should be fixed.

I'll wait couple more days for the last feedback and then merge the PR.

pcejrowski

Hey, I looked through the code and it looks very good. I'm pretty impressed by the change set. The only one think to fix IMO is the default DB.

pcejrowski · 2017-08-10T07:23:27Z

src/main/resources/reference.conf

+      # redis server: port
+      port:       6379
+      # redis server: database number
+      database:   1


Minor: database indexes start from 0, which is default to be connected when you use redis-cli

I realize that it may break some people's usage.

Honestly, I don't mind that much for backward compatibility right now as there is none right now. The change is too big to keep it compatible I think.

But on the other hand, the complexity of the new configuration is worrying me, as I posted couple secs ago.

Sure. That's the right point of time to make any breaks.

KarelCemus · 2017-08-17T20:31:58Z

@pcejrowski Thanks for your feedback. I am worried about the complexity of the configuration. I hoped for simpler settings but it conflicts with the need for expressing complex setups.

I mean that small apps with just single redis cache will have quite complex settings I think (comparing to version 1.x).

Do you have any ideas to improve the situation or do you think it is alright to keep it this way?

pcejrowski · 2017-08-17T20:38:34Z

True.
(Thinking aloud:)
You can assume, that the usage is with single server XOR cluster, thus mode parameter and different configuration subtree might be valuable. However, it may create higher configuration reading complexity.

KarelCemus · 2017-08-17T20:42:55Z

That's true, basically, I wanted to do it at the beginning. But then there raised two questions:

Would it make it more simple or more complex to comprehend? There would have to be two documentations, two examples, etc.
What to do when the user declares both types of the configuration. Furthermore, one of them should be active by default to ease the initial use thus the other one would always conflict with the first one.

I concluded that it would make the things much more difficult to understand instead of easing the use.

What do you think?

pcejrowski · 2017-08-17T20:43:12Z

Just realized source parameter

KarelCemus · 2017-08-17T20:46:03Z

Just realized source parameter

Yes, right. I don't like it but didn't figure out any other way to overcome the limitations I mentioned and support multiple types of setup.

But still, it is very complex for the simple situations. Consider this:

play.cache.redis.instances.play {
  host: ...
  port: ...
  database: ...
}

I think this is the most simple use case but I very dislike the key, it does not seem intuitive at first.

pcejrowski · 2017-08-17T20:52:55Z

Another possible solution I can think of is as follows:

for single server:

play.cache.redis {
    host: ...
    port: ...
}

for cluster

play.cache.redis {
    named-cache: true
    instances.play {
   ...
   }
}

with providing named-cache: false, which simplifies 99% of use-cases, right? (Named cache was requested after a long time of successful usage of play-redid in many projects.)

KarelCemus · 2017-08-17T20:57:49Z

So basically you propose another "source" parameter switching the configuration trees.

By default, when name-cache is false, play.cache.redis.instances.play is projected to play.cache.redis. Otherwise it is ignored.

I think that might work and would not complicate either the comprehension or the implementation as it is basically just projection. Furthermore, as I proposed the settings a bit "recursively", it works smoothly with that. In the case of name-cache false the default settings would equal to settings of the default cache.

Furthermore, in 99% cases, the migration would be quite simple I believe.

I am surprised it didn't occur to me earlier, it is quite obvious and elegant. I really like it, thanks!

pcejrowski · 2017-08-17T21:03:21Z

Exactly.
The bad thing here is defined by keyword: another.. I don't like many flags/toggles solutions, but it might be reasonable here. Have you thought about any other potential dimensions (server/cluster/connection string, named/not-named, etc.) that may come out in the future?

I tried to think of it right now and it seems, that with those two flags you cover all the specificities.

KarelCemus · 2017-08-17T21:13:00Z

I tried to think of it right now and it seems, that with those two flags you cover all the specificities.

Well, personally, I use it in a very simple scenario, so I'm not aware of all possibilities of the redis. But I cannot think of anything I know what would not be supported by these two switches.

It supports standalone/cluster/cloud as well as single cache/multiple named caches.

I can imagine introduction of other values of source property, but the mechanism and the configuration it self would remain intact, so I should cover everything available right now.

pcejrowski · 2017-08-17T21:20:39Z

That sounds pretty good. One main knob per dimension, that turns on subtree. That would be really straightforward to use it that way. Familiar, thus intuitive.

Maybe @checat @rval @solicode can add more input.

rval · 2017-08-24T00:27:34Z

Apologies for the late response.

I think both options work well, but I have a slight preference for the original instances.name approach over the alternative; I think having named-cache change the configuration tree is more confusing than adding instances.play into its hierarchy. I also like the consistency in configuring both instances.play and instances.some-name the same way.

I assume this new release will break backwards compatibility one way or another given the other changes planned. I think most integrators would accept some minor work to get on the latest release with its improvements, particularly if those are documented in UPGRADE-2.0.md.

Another thought—if instances.default is the actual default, we could remove the default-cache parameter (potential downside, what if someone actually wants a named cache of "default"?). I also think play.cache.redis.instances.default is quite clear for new and existing users.

play.cache.redis {
    instances {
        default: { # No need for default-cache since this is the default.
            host: "some.host",
            port: 1234
        },
        session-cache: {
            host: "some.other.host",
            port: 4321
        }
    }
}

Like I said, just personal preference. Either approach works for me.

johnsiu · 2017-08-24T20:59:04Z

If two named caches were configured to point to the same Redis host, port, and database, then it would be possible to have key collision.

Is it possible to add an option for named cache to implicitly prepend the namespace to the key?

akin to play2-memcached
https://github.com/mumoshu/play2-memcached/blob/master/plugin/src/main/play_2.6/com/github/mumoshu/play2/memcached/MemcachedCacheApi.scala#L35

rval · 2017-08-24T22:15:27Z

@johnsiu that crossed my mind also, but I wonder if a warning in the comments of reference.conf and/or README.md might be sufficient? This post lists a few reasons to avoid namespacing in Redis.

johnsiu · 2017-08-24T23:32:54Z

@johnsiu that crossed my mind also, but I wonder if a warning in the comments of reference.conf and/or README.md might be sufficient? This post lists a few reasons to avoid namespacing in Redis.

@rval
Here are the two reasons listed by the post:

Namespacing increases the size of every key by the size of the prefix.

This seems quite minor to me, assuming namespace is small.

You don't get to tune Redis for the individual needs of "cache" and "transactional"

I am suggesting to add namespace as an option, which can be enabled/disabled for each named cache. We are still able to configure and tune individual cache as needed.

Namespace works quite naturally with named cache since the "name" of the cache itself is the namespace.
The common use case is that a single Play application has a single cache need, but has logical division into modules, which are developed by different teams.

The different options to avoid key collision are:

spin up multiple Redis instances.
Hard to find a business justification for the operational overhead/cost.
use different Redis databases.
Deprecated and not supported by Redis Cluster.
co-ordination between teams.
low tech and error-prone.
use namespace.

I would argue that 4 is quite a reasonable option.

KarelCemus · 2017-08-25T05:56:20Z

Hi guys, thanks for your contribution.

@johnsiu I created a new ticket #118 for this discussion to keep this thread focused.

having named-cache change the configuration tree is more confusing than adding instances.play into its hierarchy

Well it depends, I think not everybody is familiar with named caches so force them to "know them" does not seem right, though the configuration is still simple. I think it might be intimidating for new users and could increase the initial barrier. Thus lowering it by keeping the default config simple in 95% cases seems right.

I assume this new release will break backwards compatibility one way or another given the other changes planned.

That's true, but I think it will break only configuration compatibility. Otherwise, it should be source compatible for regular users (not those who somehow extend or modify the core).

I think most integrators would accept some minor work to get on the latest release with its improvements, particularly if those are documented in UPGRADE-2.0.md.

I agree.

Another thought—if instances.default is the actual default, we could remove the default-cache parameter (potential downside, what if someone actually wants a named cache of "default"?). I also think play.cache.redis.instances.default is quite clear for new and existing users.

Well, it occurred to me too, but I decided to remain consistent with Play, which uses play cache as a default in its EhCache implementation.

rval · 2017-09-08T21:09:47Z

@KarelCemus all your counterpoints are totally reasonable. Like I said, just my own personal preference, certainly not a deal breaker.

Please let me know if there's anything I can do to help.

KarelCemus · 2017-09-09T09:22:40Z

@rval Thanks for your feedback, really appreciate it. Unfortunately, I must apologize for this pending PR, I have really busy 3 weeks and had no time to deal with this. One week left and it should be better again. Then I'm about to complete this PR. I have a pretty clear vision how to finish it.

Thanks for your patience.

The value should have a `-` prefix

…tion mechanisms - advanced examples and description in reference.conf - implemented a fallback cache manager

KarelCemus force-pushed the named branch 2 times, most recently from f2a612d to 06be74c Compare August 8, 2017 15:09

pelepelin reviewed Aug 8, 2017

View reviewed changes

rval approved these changes Aug 9, 2017

View reviewed changes

KarelCemus force-pushed the named branch from 250350b to e91c652 Compare August 10, 2017 06:51

pcejrowski suggested changes Aug 17, 2017

View reviewed changes

pcejrowski approved these changes Aug 17, 2017

View reviewed changes

KarelCemus mentioned this pull request Aug 25, 2017

Key prefix with named caches #118

Closed

KarelCemus added 8 commits September 18, 2017 16:31

Redesigned play-redis configuration

39d47ff

Upper layers of play-redis considers named instances

dd691f4

removed unused files

79eccfc

Refactored components and modules into separate files

09879cb

optionally bind default apis

44845bb

Fixed naming in reference.conf

7bfa083

Fix deprecation option value in scalacOptions (#117)

e469fb4

The value should have a `-` prefix

Bumped Play version and fixed rebase

0e0d6a8

KarelCemus force-pushed the named branch from b85a769 to b9acd75 Compare September 24, 2017 12:47

reference.conf provides both simple fallback and advance cache defini…

3bb9f53

…tion mechanisms - advanced examples and description in reference.conf - implemented a fallback cache manager

KarelCemus force-pushed the named branch from b9acd75 to 3bb9f53 Compare September 24, 2017 13:29

KarelCemus merged commit 0970e17 into master Sep 24, 2017

KarelCemus deleted the named branch September 24, 2017 13:59

Named cache #114

Named cache #114

Uh oh!

Conversation

KarelCemus commented Aug 8, 2017

Uh oh!

pcejrowski commented Aug 8, 2017

Uh oh!

KarelCemus commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarelCemus commented Aug 9, 2017

Uh oh!

rval left a comment

Choose a reason for hiding this comment

Uh oh!

rval Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarelCemus commented Aug 10, 2017

Uh oh!

pcejrowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarelCemus commented Aug 17, 2017

Uh oh!

pcejrowski commented Aug 17, 2017

Uh oh!

KarelCemus commented Aug 17, 2017

Uh oh!

pcejrowski commented Aug 17, 2017

Uh oh!

KarelCemus commented Aug 17, 2017

Uh oh!

pcejrowski commented Aug 17, 2017

Uh oh!

KarelCemus commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcejrowski commented Aug 17, 2017

Uh oh!

KarelCemus commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcejrowski commented Aug 17, 2017

Uh oh!

rval commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnsiu commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rval commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnsiu commented Aug 24, 2017

Uh oh!

KarelCemus commented Aug 25, 2017

Uh oh!

rval commented Sep 8, 2017

Uh oh!

KarelCemus commented Sep 9, 2017

Uh oh!

KarelCemus commented Aug 8, 2017 •

edited

Loading

rval Aug 9, 2017 •

edited

Loading

KarelCemus commented Aug 17, 2017 •

edited

Loading

KarelCemus commented Aug 17, 2017 •

edited

Loading

rval commented Aug 24, 2017 •

edited

Loading

johnsiu commented Aug 24, 2017 •

edited

Loading

rval commented Aug 24, 2017 •

edited

Loading