Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

pkg/idempotent: create redis implementation #21

Closed
adamdecaf opened this issue Oct 23, 2018 · 5 comments
Closed

pkg/idempotent: create redis implementation #21

adamdecaf opened this issue Oct 23, 2018 · 5 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@adamdecaf
Copy link
Member

From the X-Idempotency-Key issue (#15 (comment)) we want to implement a redis implementation of idempotent.Recorder.

Can we run an inmem redis for tests? I don't know off hand what's available.

@adamdecaf adamdecaf added help wanted Extra attention is needed good first issue Good for newcomers labels Oct 23, 2018
u5surf added a commit to u5surf/paygate that referenced this issue Oct 25, 2018
u5surf added a commit to u5surf/paygate that referenced this issue Oct 25, 2018
@u5surf
Copy link

u5surf commented Oct 25, 2018

@adamdecaf
Hi. I implemented idempotent with redis (using github.com/gomodule/redigo/redis)
Refer to #15 ,I also add 1 unittest and transfers_test.
result:

$go test -v ./...
=== RUN   TestAccountType
--- PASS: TestAccountType (0.00s)
=== RUN   TestAccountType__json
--- PASS: TestAccountType__json (0.00s)
=== RUN   TestAmount
--- PASS: TestAmount (0.00s)
=== RUN   TestAmount__FromString
--- PASS: TestAmount__FromString (0.00s)
...
=== RUN   TestTransfers__transferRequest
--- PASS: TestTransfers__transferRequest (0.00s)
=== RUN   TestTransferType__json
--- PASS: TestTransferType__json (0.00s)
=== RUN   TestTransferStatus__json
--- PASS: TestTransferStatus__json (0.00s)
=== RUN   TestTransfers__idempotencyLRU
--- PASS: TestTransfers__idempotencyLRU (0.00s)
=== RUN   TestTransfers__idempotencyRedis
--- PASS: TestTransfers__idempotencyRedis (0.01s)
=== RUN   TestTransfers__getUserTransfers
--- PASS: TestTransfers__getUserTransfers (0.01s)
PASS
ok  	github.com/moov-io/paygate	0.155s
?   	github.com/moov-io/paygate/internal/version	[no test files]
=== RUN   TestACH__pingRoute
2018/10/25 12:06:11 http: multiple response.WriteHeader calls
--- PASS: TestACH__pingRoute (0.00s)
=== RUN   TestACH__buildAddress
--- PASS: TestACH__buildAddress (0.00s)
=== RUN   TestACH__addRequestHeaders
--- PASS: TestACH__addRequestHeaders (0.00s)
=== RUN   TestACH__retryWait
--- PASS: TestACH__retryWait (0.00s)
=== RUN   TestACH__retry
--- PASS: TestACH__retry (0.04s)
PASS
ok  	github.com/moov-io/paygate/pkg/achclient	0.064s
?   	github.com/moov-io/paygate/pkg/idempotent	[no test files]
=== RUN   TestLRU
--- PASS: TestLRU (0.00s)
PASS
ok  	github.com/moov-io/paygate/pkg/idempotent/lru	0.010s
=== RUN   TestLRU
--- PASS: TestLRU (0.01s)
PASS
ok  	github.com/moov-io/paygate/pkg/idempotent/redis	0.035s

But I had not considered how can we get started redis server(though, Is getting to start server scope?), proper error handling of dial and send to it(now I use panic when catch the error, I suppose to be handling as log to returns error from SeenBefore func), and a mechanism to be switching LRU which you implement and Redis yet. Don't you have any ideas following these issues?

@adamdecaf
Copy link
Member Author

Thanks @u5surf!

There's https://github.com/rafaeljusto/redigomock which looks like it works with the redis library chosen too.

@wadearnold That project is GPL v2, can we use that? Probably not?
https://github.com/rafaeljusto/redigomock/blob/master/LICENSE

We defiantly don't want to panic. Returning the error with whatever context we can is helpful.

Reading the diff I noticed there's no TTL command used. We should call that as part of SET <key> <value>. The default timeout should be 24 hours. (Our docs state that already.

u5surf added a commit to u5surf/paygate that referenced this issue Oct 25, 2018
u5surf added a commit to u5surf/paygate that referenced this issue Oct 25, 2018
@u5surf
Copy link

u5surf commented Oct 25, 2018

I fixed to use context to be handling redis errors. and add defaultTimeout in redis.

@adamdecaf
Copy link
Member Author

Cool! I don't think we need a context though. Can you open a PR?

@adamdecaf
Copy link
Member Author

Closing, see moov-io/base#16

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants