eko/gocache

Should Rueidis store implementation return string?

rueian opened this issue · 3 comments

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(),
		)
	}
}
  1. 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

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

Thank you for this implementation.

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

Hi @eko,

Thank you very much. Actually, I would also like to add an rueidis example to readme: #189