Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: Cache Responses #390

Closed
anarcher opened this issue Sep 14, 2018 · 14 comments · Fixed by #634
Closed

API: Cache Responses #390

anarcher opened this issue Sep 14, 2018 · 14 comments · Fixed by #634
Assignees
Labels

Comments

@anarcher
Copy link
Contributor

No description provided.

@anarcher anarcher added the API label Sep 14, 2018
@spikeekips
Copy link
Contributor

spikeekips commented Oct 24, 2018

I can fill the missing description of this issue :)

  • Most of API relies on storage, we need to reduce the usage of storage

This is checklist.

  • BlockXX records should be cached; except BlockAccount
  • Caching BlockAccount records and it's expiration can be 1s for considering very frequent requests
  • 404 also should be cached, but it's expiration can be 1s or 2s
  • Cache server can be embedded as memory type
  • Cache should support external cache server like 'memcached', 'redis', or 'AWS elastic cache', etc.

@kfangw
Copy link
Contributor

kfangw commented Oct 24, 2018

IMO, Redis would be satisfying the requirements.
On the Redis, It is very easy to use and set the TTL.

@spikeekips
Copy link
Contributor

@kfangw Yes, redis will be good. There are some libraries for various kind of the existing cache system, https://github.com/avelino/awesome-go .

@anarcher
Copy link
Contributor Author

The cache responses are handled by URL-based? If it is, how about CacheMiddleware?

@spikeekips
Copy link
Contributor

spikeekips commented Oct 24, 2018

@anarcher: The cache responses are handled by URL-based? If it is, how about CacheMiddleware?

It will be good, if we can manage different cache policy by handler, as I described BlockAccount should be managed differrently.

@anarcher
Copy link
Contributor Author

anarcher commented Oct 25, 2018

cacheMiddleware := cache.NewMiddleware(
   cache.Option{
       Adapter: "mem",
       AdapterOption: "max=1000,expire=2s",
       MatchURLPrefix : "/v1/api/account", 
   },
   cache.Option{
       Adapter: "redis",
       AdapterOption: "endpoint=127.0.0.1:11211,max=1000",
       MatchURLPrefix : "/", 
   },...
}

@anarcher
Copy link
Contributor Author

anarcher commented Oct 31, 2018

import "boscoin.io/sebak/lib/network/http/cache"
adapter := cache.NewMemAdapter(10000)
cached  := cache.New(
    cache.WithExpire(1 *time.Second),
    cache.WithAdpater(adapter),
    cache.WithStatusCode(200), // infinite
    cache.WithStatusCode(201,10 * time.Minute), 
    cache.WithStatusCode(404,3 * time.Minute),
    cache.WithMethods("GET","HEAD"),
)
nr.network.AddHandler(
    apiHandler.HandlerURLPattern(api.GetTransactionOperationsHandlerPattern),
    cached.Middleware(apiHandler.GetOperationsByTxHashHandler),
).Methods("GET", "OPTIONS")

@spikeekips @Geod24 @Charleslee522 I would like to prefer this approach for cache response.
What do think?

@anarcher anarcher self-assigned this Oct 31, 2018
@anarcher anarcher added this to ToDo in Iteration 18-06 via automation Oct 31, 2018
@Geod24
Copy link
Contributor

Geod24 commented Oct 31, 2018

@anarcher :

  • What's the unit of 10000 (line 2) ?
  • Expire at 1s is very low, isn't it ?
  • What are the two status codes for ?

Other than that, it looks pretty expressive 👍

@anarcher
Copy link
Contributor Author

  • What's the unit of 10000 (line 2) ?
    Yes, max item limit.

  • Expire at 1s is very low, isn't it ?
    It's an example or sample. and reference it

Caching BlockAccount records and it's expiration can be 1s for considering very frequent requests

  • What are the two status codes for ?
    Reference it :

404 also should be cached, but it's expiration can be 1s or 2s

@Geod24
Copy link
Contributor

Geod24 commented Oct 31, 2018

IMHO, LRU+ observer based eviction is better performance than time-based

I agree with this, and I think it's also more correct.
This this, I expect that the expiration can be fairly high (hours).

@anarcher
Copy link
Contributor Author

anarcher commented Oct 31, 2018

I agree with this, and I think it's also more correct.
This this, I expect that the expiration can be fairly high (hours).

I would like to do time expire based. (Not observer)

@Geod24
Copy link
Contributor

Geod24 commented Oct 31, 2018

I would like to do time expire based.

What changed your mind ? Also, what is the strategy to avoid serving stale data ?

@anarcher
Copy link
Contributor Author

Caching BlockAccount records and it's expiration can be 1s for considering very frequent requests

@anarcher anarcher removed this from ToDo in Iteration 18-06 Nov 1, 2018
@anarcher anarcher mentioned this issue Nov 2, 2018
5 tasks
@anarcher anarcher added this to ToDo in Iteration 18-06 via automation Nov 2, 2018
@spikeekips spikeekips moved this from ToDo to Doing in Iteration 18-06 Nov 6, 2018
Iteration 18-06 automation moved this from Doing to Done Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants