Various improvements to Redis providers

[ReubenBond]

• Use consistent configuration pattern
• Add/update configuration validators
• Use unique key prefixes for all providers
• Add expiry to all keys for testing
• Consistently throw serializable exceptions
• Update doc comments
• Mark classes which do not need to be public as internal
• Avoid clobbering the entire Redis instance during test runs
• Other minor cleanups

Note: this change breaks backwards compatibility with the existing Redis grain directory. It also breaks compatibility with the OrleansContrib redis providers. I think this is acceptable, since it is a new set of providers and grain directory entries become invalidated after a full cluster upgrade anyway. cc @benjaminpetit

Also note that all Redis providers are marked as -beta1 currently. Should we remove that label?

I do not consider the Reminders provider suitable for large scale use since it puts all values into a single hash set (with one key in the hash set per reminder entry). This is fine for single instance Redis servers, but it is likely not ideal for Redis Cluster. Should we aim to update this before marking it as stable?

cc @mgravell & @willg1983

[EPinci]

Also note that all Redis providers are marked as -beta1 currently. Should we remove that label?

I do not consider the Reminders provider suitable for large scale use since it puts all values into a single hash set (with one key in the hash set per reminder entry). This is fine for single instance Redis servers, but it is likely not ideal for Redis Cluster. Should we aim to update this before marking it as stable?

I initially applied the -beta1 tag because the existing directory provider was marked as such.

The compatibility break with the OrleansContrib was already known due to the serialization changes.
Thinking about this, should also that repo be marked as obsolete once these providers are released?

I would say that the change to make it suitable for a large cluster will probably break compatibility with the current, so we should probably aim for that in this block in order not to change it (again) afterwards.

@ReubenBond let me know if I can be of any help in here.

[mgravell]

if we're updating a single item, are we sure we want to do this by score? zrem may be simpler than zremrangebyscore, and can probably just do as a single zadd which (unless incr is specified) effectively does a re-insert with the new score anyway

[ReubenBond]

I don't quite understand this comment, sorry. What are you proposing in terms of API calls?

[mgravell]

let me just check my understanding of the setup here first; so a redis sorted set is an order set of token + score pairs (ordered by score then the token); here, the SortedSetRemoveRangeByValueAsync maps to zremrangebylex, which removes a range by token (which is the non-performant axis - i.e. the non-sorted axis) - but it looks like we're really just updating a single entity here? now; I can't tell without squinting really hard, but: is the value here changing? if not, we can just issue the _db.SortedSetAddAsync(_hashSetKey, value, 0), which will effectively just:

• locate the item with key value if it exists, and change the score to 0, effectively moving the item in the sorted set
• if it doesn't exist, just add value with score 0

and if we're just issuing one command, we don't need the transaction as there is no atomicity concern

If I've misunderstood what we're storing here and the data we are removing isn't keyed with value, then: fine - my bad

---

unrelated; I don't know what .Ignore() does, but whatever it is: you can probably just use:

_ = _db.SortedSetRemoveRangeByValueAsync(_hashSetKey, from, to, flags: CommandFlags.FireAndForget);

which will cut out a few bits of internal machinery (and never fault)

[ReubenBond]

which is the non-performant axis - i.e. the non-sorted axis

I believe you, but what is your source for this? The Redis docs state the time complexity for both as being O(Log(N)+M)

unrelated; I don't know what .Ignore() does, but whatever it is: you can probably just use:

The method returns a Task, so any exception which might be thrown by it is being swallowed by that .Ignore() extension method. We do care that it succeeds, but that should be propagated to the transaction. What is the right way to do this?

is the value here changing?

Yes, the method is at least updating the ETag component of the value (which is a JSON-encoded sequence). It is likely also updating other parts of the value (eg, the StartAt or Period properties)

[ReubenBond]

Ideally, what we want is:

• To be able to perform range queries on a key value (hash code of grain id + grain id)
  • Currently, we are using a lexicographic sort with the format [HashCode,GrainId,ReminderName,ETag,...other elements...]
• To be able to ensure that if an ETag is provided, that the operation only proceeds if any existing entry matches that ETag (it is part of the key value)

Is there a way that we can add a transaction condition that there must be at least one value in the range [from,to]? I am hoping we don't need to resort to Lua, but perhaps we do.

[ReubenBond]

I think we should use a Lua script instead of a transaction...

[mgravell]

Lua is almost always both simpler and more efficient than the WATCH/MULTI/EXEC beast; I support this notion

[mgravell]

thoughts added

[ReubenBond]

I attempted to address your feedback, many thanks, @mgravell!