Add module for transactions cache #440
Conversation
Add a transactions module in dendrite/common. This is needed for idempotent APIs. Signed-off-by: Anant Prakash <anantprakashjsr@gmail.com>
|
|
||
| // FetchTransaction looks up an entry for txnID in Cache. | ||
| // Returns a JSON response if txnID is found, else returns error. | ||
| func (t *Cache) FetchTransaction(txnID string) (*util.JSONResponse, error) { |
There was a problem hiding this comment.
I suspect the API will be easier to use if we return a bool instead of an error
| func cacheCleanService(t *Cache) { | ||
| for { | ||
| time.Sleep(CleanupPeriod) | ||
| go clean(t) |
There was a problem hiding this comment.
I don't think there is much reason to spawn a goroutine here, it doesn't matter if we're imprecise about the timings so its fine to block the loop while we do the clean up.
(FWIW, if we did care about getting a steady rate then we shouldn't use a loop with time.Sleep, as its very easy for the timings to start to drift. In go, you'd probably use time.Ticker)
|
|
||
| func clean(t *Cache) { | ||
| expire := time.Now().Add(-CleanupPeriod) | ||
| for key := range t.txns { |
There was a problem hiding this comment.
This loop over t.txns needs to be in the lock too.
| func TestConcurentAccess(t *testing.T) { | ||
| fakeTxnCache := New() | ||
| // Signal that this test should run in parallel | ||
| t.Parallel() |
There was a problem hiding this comment.
This only tells the test runner it can run this test in parallel with other tests (I believe), so this doesn't do what you want.
| for i := 1; i <= 1000; i++ { | ||
| go check(t, fakeTxnCache, i) | ||
| } | ||
| } |
There was a problem hiding this comment.
Honestly, I'm not sure this test is really testing anything much; if there is a concurrency bug then this is unlikely to pick it up. The concurrency model here is to simply use RWLock, so I think its safe to simply not have a unit test and to manually make sure we lock in the appropriate methods.
(If the cache was more complicated, then we'd probably want to add hooks into the cache to let us write deterministic tests.)
| ) | ||
|
|
||
| // CleanupPeriod represents time in nanoseconds after which cacheCleanService runs. | ||
| const CleanupPeriod time.Duration = 30 * time.Minute |
There was a problem hiding this comment.
Personally I would make this an option for Cache
|
Thinking about it, I wonder if we should be a little concerned about the clean up method. We'll need to lock for during iteration of the map, which could take a little while if the map is large. It's probably fine for now, but an alternative trick that you can use to make the clean up efficient is to:
This works by effectively rotating the maps. A transaction will get added to the first map, it will at some point get rotated to the second map, and finally (after at least N minutes) the map with the transaction gets dropped. |
Signed-off-by: Anant Prakash <anantprakashjsr@gmail.com>
Signed-off-by: Anant Prakash <anantprakashjsr@gmail.com>
| // New creates a new Cache object, starts cacheCleanService. | ||
| // Takes cleanupPeriod (in minutes) as argument, in case of 0 DefaultCleanupPeriod is used instead. | ||
| // Returns a reference to newly created Cache. | ||
| func New(cleanupPeriodInMins int) *Cache { |
There was a problem hiding this comment.
Given we have a time.Duration type, I'd use that. If you want to make it optional then either take a pointer and check for null or have New() and NewWithPeriod(..)
There was a problem hiding this comment.
In most scenario one would let the Default be as it is, passing nil for that case is cumbersome.
So I will avoid the nil-able pointer parameter.
| // Entries are evicted after a certain period, defined by cleanupPeriod. | ||
| type Cache struct { | ||
| sync.RWMutex | ||
| txnsMaps [2]txnsMap |
There was a problem hiding this comment.
I'd probably give a quick comment here to say why there are two maps, and how this all works internally
Add code comments Signed-off-by: Anant Prakash <anantprakashjsr@gmail.com>
Add a transactions module in dendrite/common. This is needed for idempotent APIs.
based on #419.