3 ms·

This is strange to me. Did Github do client-based sharding because they were trying to get around the upfront key enumeration limitation in Lua scripts? Why did

by junon 6y ago

This is strange to me. Did Github do client-based sharding because they were trying to get around the upfront key enumeration limitation in Lua scripts? Why didn't they use the cluster's ability to proxy requests to the appropriate sharded worker? As-is, they could have just passed `rate_limit_key+':exp'` as a second KEYS entry and it would have ensured the key existed for operation. They were deriving keys off of apriori information, so they could have just as easily foregone the client-side complexity and just put the redis cluster in a sharded configuration. I wonder what sorts of performance impact this had (the page doesn't mention it). Client-side sharding almost certainly increased the codebase complexity and it doesn't seem like they measured any real impact from doing it this way (or maybe they just chose not to report it).

tlhunter 6y ago
I was concerned about the missing KEYS entry, too. I believe the script would fail if the two keys were on different machines, assuming both key names were provided as KEYS, though the {} syntax should have avoided that. By generating the key name in a Lua script it forces the two related keys to be on the same machine. At the end of the day, calculating the server client side isn't necessarily messy. Surely the Lua script deserves a warning about the key name being generated within the script. I think the ideal script would use two keys, the first being like `foo-{1234}`, and the second `foo-{1234}:exp`, with both key names being provided via KEYS. Then the native Redis clustering should work.
- junon 6y ago
  > By generating the key name in a Lua script it forces the two related keys to be on the same machine. Perhaps I'm misunderstanding what you're saying, but this isn't true. You have to supply ALL keys that you wish to read from in the KEYS array prior to running scripts in a sharded setup, otherwise Redis will fail as it can't lock the map long enough to perform potentially-failing I/O operations to fetch them. I suppose it could, but it wouldn't make any sense to and would violate a bunch of otherwise strong and useful guarantees. > Surely the Lua script deserves a warning about the key name being generated within the script. My point is they're able to guarantee the key is present simply because they're using client-side sharding with Redis in replica/sentinel (or even standalone) modes, thus you are free to construct keys within scripts without issue. Since they're owning the `:exp` key within the script, then the `:exp` keys are guaranteed to be within the same instance. > I think the ideal script would use two keys, the first being like `foo-{1234}`, and the second `foo-{1234}:exp`, with both key names being provided via KEYS. Then the native Redis clustering should work. This isn't ideal if the `:exp` key causes high colocation misses, causing extra I/O ops for each script execution (since they would have to be fetched prior to the script execution). Also keep in mind that Redis has multiple clustering modes, so you should specify which you mean. Lastly, I've never seen `{}` in keys, only `:`, just a nitpick. I don't think there's any benefit there except for extra bytes over the wire, as most people aren't putting un-sanitized user input in Redis keys (I'd hope not).

by junon 6y ago

tlhunter 6y ago

I was concerned about the missing KEYS entry, too. I believe the script would fail if the two keys were on different machines, assuming both key names were provided as KEYS, though the {} syntax should have avoided that. By generating the key name in a Lua script it forces the two related keys to be on the same machine. At the end of the day, calculating the server client side isn't necessarily messy. Surely the Lua script deserves a warning about the key name being generated within the script. I think the ideal script would use two keys, the first being like `foo-{1234}`, and the second `foo-{1234}:exp`, with both key names being provided via KEYS. Then the native Redis clustering should work.

junon 6y ago
> By generating the key name in a Lua script it forces the two related keys to be on the same machine. Perhaps I'm misunderstanding what you're saying, but this isn't true. You have to supply ALL keys that you wish to read from in the KEYS array prior to running scripts in a sharded setup, otherwise Redis will fail as it can't lock the map long enough to perform potentially-failing I/O operations to fetch them. I suppose it could, but it wouldn't make any sense to and would violate a bunch of otherwise strong and useful guarantees. > Surely the Lua script deserves a warning about the key name being generated within the script. My point is they're able to guarantee the key is present simply because they're using client-side sharding with Redis in replica/sentinel (or even standalone) modes, thus you are free to construct keys within scripts without issue. Since they're owning the `:exp` key within the script, then the `:exp` keys are guaranteed to be within the same instance. > I think the ideal script would use two keys, the first being like `foo-{1234}`, and the second `foo-{1234}:exp`, with both key names being provided via KEYS. Then the native Redis clustering should work. This isn't ideal if the `:exp` key causes high colocation misses, causing extra I/O ops for each script execution (since they would have to be fetched prior to the script execution). Also keep in mind that Redis has multiple clustering modes, so you should specify which you mean. Lastly, I've never seen `{}` in keys, only `:`, just a nitpick. I don't think there's any benefit there except for extra bytes over the wire, as most people aren't putting un-sanitized user input in Redis keys (I'd hope not).