Support for Redis storage #3

payomdousti · 2019-02-28T00:17:52Z

No description provided.

…ability

…tor function

…tistics aren't in scope for implementation.

…or better readability

…testing at this point.

… purpose, removing from tests.

…k ties by lex.

…integers.

…ould be contained in a list.

…anging get range to be index_jumps_before_entry..index_jumps_after_entry

…mmit.

…ehavor from negative pointers in redis ZRANGE.

…legibility.

…ince it's our only way to ensure we have the correct indices for each element.

maxim

Hello Payom, this is excellent work. 🏆

I would love to add Redis support to this library, and you seem to have built a solid implementation. I'm sorry it took me a while to take a look, but this is quite a lot of code. If you could spare some more time on this, I would also appreciate it if you could:

Clean up the commit history by squashing most of the commits into just 1-3 commits (depending on what makes sense) to be more of a clean addition of the Redis service. Please keep the commit subject under 50 characters, and combine the description of various considerations you made into a single commit message body (within 80 character line length). I.e. one way would be to go over your commits, collect various thoughts you put into them into a single summary, and make this the body of the squashed commit.
Make the travis build pass.

Thank you again for contributing your work. 👍

maxim · 2019-06-08T19:25:21Z

lib/cx_leaderboard/application.ex

@@ -0,0 +1,18 @@
+defmodule CxLeaderboard.Application do


Could you clarify why is the application file needed? I haven't programmed elixir for about a year now, so I might be rusty on some concepts, but if this lib worked without application, why does Redis make it necessary?

This was so we could run the redix process on boot.

maxim · 2019-06-08T19:26:14Z

mix.exs

@@ -38,7 +39,8 @@ defmodule CxLeaderboard.MixProject do
    [
      {:dialyxir, "~> 0.5", only: :dev, runtime: false},
      {:benchee, "~> 0.12", only: :dev, runtime: false},
-      {:ex_doc, "~> 0.18", only: :dev, runtime: false}
+      {:ex_doc, "~> 0.18", only: :dev, runtime: false},
+      {:redix, ">= 0.0.0"}


Is there any way we can make redix not a dependency unless someone wants to use Redis store?

I'm not as familiar with idiomatic elixir, but i can take a look!

maxim · 2019-06-08T19:28:00Z

test/redis_storage_case.exs

@@ -0,0 +1,289 @@
+defmodule CxLeaderboard.RedisStorageCase do


Is there a reason why RedisStorageCase is needed as opposed to reusing the existing StorageCase?

yeah, so this is the tip of the iceberg with this PR, and I'm curious to get your thoughts. The Redis implementation necessitated some differences in semantics since sorting does not happen as a separate operation (it happens atomically as things are added to the sorted set). There are also some other important changes that aren't top of mind for me, but would be happy to point out once i take a closer look again.

To answer your question though, it was a clean way of pursuing this branch without breaking existing functionality. I think you're right that in the future this could go away potentially.

Ah, I see. I'm happy to re-evaluate what we're testing for. For example, I notice you left a comment that it doesn't make sense to get a range in reverse order.

My goal here is to try and make the public interface more or less consistent, and purely Leaderboard focused, regardless of storage engine. I would rather we remove some of the not-very-useful functionality (like you did with the reverse range test) than have different storage engines provide different behavior.

Would you mind going over each alteration you had to make between this and default StorageCase, and providing some reasons why each was necessary? In the end it might make sense to clean up StorageCase to accommodate RedisStore better.

maxim · 2019-06-08T19:30:09Z

.gitignore

@@ -22,3 +22,6 @@ erl_crash.dump
 # Ignore package tarball (built via "mix hex.build").
 cx_leaderboard-*.tar

+\.idea


Right now we don't have any editor-specific ignores in .gitignore. Do you think moving these into your personal home directory's global git ignore would be sufficient? That way you wouldn't need to ignore these files on individual projects.

maxim · 2019-06-08T19:31:10Z

README.md

 ## Global Leaderboards

-If you want to have a global leaderboard starting at the same time as your application, and running alongside it, all you need to do is declare a worker as follows:
+If you want to have a global leaderboard starting at the same time as your application, and running alongside it, all you need to do is declare a 
+as follows:


Seems like this might be a typo.

maxim and others added 30 commits June 5, 2018 00:48

Fix README instructions for declaring a worker

1b18239

Add Documentation link to README

1845b37

redis version of leaderboard

a3ad362

implement create

7b81293

delete redis leaderboard file

afb988d

spawn redix pool process on start

8901ab5

spin up redis in application file properly

4dedfc2

rename module

b2a3cdd

stub out populate method for future implemenatation

97436a7

implement clear function

9e8675f

implement add function

20050f6

implement add function

ef478c1

make indexer optional in redisstore module

78f0d65

implement populate

cdc9175

implement async_populate

3bf0cd6

implement add

2c15306

implement update

64f321e

implement add_or_update

45a6031

implement bottom (does not yet return full record struct)

720b967

implement count

b035ff9

implement top (does not yet return full record struct)

138a5dd

remove potentially unrelated code (part 1)

7e972a8

remove potentially unrelated code (part 2)

4c393fc

remove delegation and stub get

77609a0

implement top function as a stream

a6da7b2

implement bottom function as a stream

cfbbff2

correct the return type for the count function

ac7199e

don't do anything special for create function

2187e8c

use leaderboard name

eadce16

formatting

e9d4aa9

payomdousti added 29 commits February 22, 2019 20:43

fix variable references

94040ee

refactor calls to redis with user of helper functions to improve read…

6c23ebf

…ability

move position of count function in file

0a6168c

refactor top and bottom functions to use a common redis_stream_genera…

8f01fc8

…tor function

map entries to records before streaming out

b26f90c

update module description

463d9f9

Implement get with the proper range data type

cc28bc9

delete extraneous fn inputs

93b5674

compute rank from index

35955bf

buffer size as constant for redis stream

6a6fc60

rename supervisor

fc01714

add tests specific to redis storage. some aspects such as indexer sta…

d274c85

…tistics aren't in scope for implementation.

loosen requirements for erroring

ff2435c

return empty set if nothing is found

c2d0f60

elixir style conventions

5fd18e6

cast score to integer, add entry key as default payload, and format f…

e9c7ed3

…or better readability

abandon mocking redis. will have to require local redis instance for …

f5f71b1

…testing at this point.

we do not believe it's a good idea to store objects in redis for this…

dd92334

… purpose, removing from tests.

since we don't index, remove support for tiebreakers. redis will brea…

3824181

…k ties by lex.

We don't agree with the semantics here. Scores should only be signed …

8bd6821

…integers.

we're adopting a more lenient take on ops with no diff in result set.

bf43b9e

no support for indexer in redis store.

50f11fb

no support for ties in rankings

a9afedd

we want a more consistent result set from get. we think everything sh…

124f877

…ould be contained in a list.

reference a change in semantics from test of get with range. we're ch…

0b158b7

…anging get range to be index_jumps_before_entry..index_jumps_after_entry

tests which show change in semantics as referenced in the previous co…

4261cc0

…mmit.

ensure we're only using values greater than 0 to prevent unintended b…

7d3ce59

…ehavor from negative pointers in redis ZRANGE.

extract mapping of entry to record into a helper function for better …

5418df4

…legibility.

fetch and join on the correct rank from redis with each given entry s…

a5aa9b9

…ince it's our only way to ensure we have the correct indices for each element.

maxim requested changes Jun 8, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support for Redis storage #3

Support for Redis storage #3

Uh oh!

payomdousti commented Feb 28, 2019

Uh oh!

maxim left a comment •

edited

Loading

Uh oh!

maxim Jun 8, 2019

Uh oh!

payomdousti Jun 8, 2019

Uh oh!

maxim Jun 8, 2019

Uh oh!

payomdousti Jun 8, 2019

Uh oh!

maxim Jun 8, 2019

Uh oh!

payomdousti Jun 8, 2019

Uh oh!

payomdousti Jun 8, 2019

Uh oh!

maxim Jun 8, 2019

Uh oh!

maxim Jun 8, 2019

Uh oh!

maxim Jun 8, 2019

Uh oh!

Uh oh!

		@@ -0,0 +1,289 @@
		defmodule CxLeaderboard.RedisStorageCase do

Support for Redis storage #3

Are you sure you want to change the base?

Support for Redis storage #3

Uh oh!

Conversation

payomdousti commented Feb 28, 2019

Uh oh!

maxim left a comment • 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!

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!

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!

Uh oh!

maxim left a comment •

edited

Loading