eko / gocache

☔️ A complete Go cache library that brings you multiple ways of managing your caches
https://vincent.composieux.fr/article/i-wrote-gocache-a-complete-and-extensible-go-cache-library/
MIT License
2.46k stars 195 forks source link

Should Rueidis store implementation return string? #187

Closed rueian closed 1 year ago

rueian commented 1 year ago

Hi @eko, @rwrz,

Thank you for adding rueidis integration. It is my honor that rueidis can be integrated into this popular cache library.

Just wondering why doesn't Rueidis store implementation return a simple string instead of a rueidis.RedisResult? Are there some considerations I missed? I thought It would be much friendly if it returns a string to users like what go-redis integration does.

For example:

func (s *RueidisStore) Get(ctx context.Context, key any) (any, error) {
    cmd := s.client.B().Get().Key(key.(string)).Cache()
    str, err := s.client.DoCache(ctx, cmd, s.options.ClientSideCacheExpiration).ToString()
    if rueidis.IsRedisNil(err) {
        err = lib_store.NotFoundWithCause(err)
    }
    return str, err
}

Also, I noticed that there are some room for improvement:

  1. Each tag operation in setTags can be manually pipelined:
    func (s *RueidisStore) setTags(ctx context.Context, key any, tags []string) {
    ttl := 720 * time.Hour
    for _, tag := range tags {
        tagKey := fmt.Sprintf(RueidisTagPattern, tag)
        s.client.DoMulti(ctx,
            s.client.B().Sadd().Key(tagKey).Member(key.(string)).Build(),
            s.client.B().Expire().Key(tagKey).Seconds(int64(ttl.Seconds())).Build(),
        )
    }
    }
  2. I am going to release a new CacheTTL() method this weekend. It can retrieve the remaining client-side TTL of a cached message. Then, the GetWithTTL can be just like this:
    func (s *RueidisStore) GetWithTTL(ctx context.Context, key any) (any, time.Duration, error) {
    cmd := s.client.B().Get().Key(key.(string)).Cache()
    res := s.client.DoCache(ctx, cmd).Cache(), s.options.ClientSideCacheExpiration)
    ttl := res.CacheTTL()
    str, err := res.ToString()
    if rueidis.IsRedisNil(err) {
        err = lib_store.NotFoundWithCause(err)
    }
    return str, time.Duration(ttl) * time.Second, err
    }

What do you think? I am happy to open a PR for the above changes. 😊

eko commented 1 year ago

Hello @rueian,

I agree with your proposals, if you want to open a PR you're mostly welcome, elsewhere I could find some time on next week to do these changes.

Thank you for this and to have built Rueidis :)

eko commented 1 year ago

Thank you for this implementation.

Closing this PR for now but feel free to reopen if you still see anything that can be improved.

rueian commented 1 year ago

Hi @eko,

Thank you very much. Actually, I would also like to add an rueidis example to readme: https://github.com/eko/gocache/pull/189