Skip to content
13 changes: 12 additions & 1 deletion pkg/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,20 @@ import (
)

var (
ErrNotFound = errors.New("not found")
ErrNotFound = errors.New("not found")
ErrUnimplemented = errors.New("unimplemented")
)

// Getter wraps a method to read from cache.
type Getter interface {
Get(key interface{}) (interface{}, error)
GetAll() ([]interface{}, error)
}

// Putter wraps a method to write to cache.
type Putter interface {
Put(key interface{}, value interface{}) error
PutHash(key interface{}, value interface{}) error

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why do we need this? I think we can reuse Put function.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case we want to use RedisHashCache.Put(...) interface, I think we should keep the existing Put implementation for RedisHashCache. 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case we want to use RedisHashCache.Put(...) interface

Can you explain more about this? That is for putting a set of (field, value) for a key?

@khanhtc1202 khanhtc1202 Jul 7, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is for putting a set of (field, value) for a key?

Since the current RedisHashCache has embedded RedisCache instance, in the caller side, we can use RedisHashCache.Get(key) which call to RedisHashCache.RedisCache.Get(key) with the current implementation. In that case, It will return an error not found (even if we Put the (k,v) pair before) if we override the Put interface with our PutHash implementation. That why I think we should keep the existing Put interface and add PutHash which be used for the new purpose of storing (k,v) as values of the defined hashkey of RedisHashCache.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not embed RedisCache. We can either add a new cache: RedisCache field or use redis.Redis directly.
I think we have no reason to put PutHash function to all cache interfaces.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if I understand correctly, I feel like the current RedisCache is enough. We can just use piped-key as a key and use a stats byte sequence as a value. Tell me why using HSET if any.

@nghialv nghialv Jul 7, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is why GetAll and RedisHashCache were introduced. We can have all stats of all pipeds by a single command in a fast way to expose for prometheus.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙆‍♀️ Get you guys' point 🙏
Addressed on 478ee0d 👀
At this implementation, I change all implementation for Cache interface of RedisHashCache as it will treat the Hash object stored in redis as its backend and replace all XXX commands with HXXX commands.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, makes sense. it's for retrieving all stats in bulk.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell me why using HSET if any.

With only the single SET interface, we need a way to know all possible piped (ID) currently registered to the controlplane and query for the data one by one which much slower, I guess 👀

}

// Deleter wraps a method to delete from cache.
Expand Down Expand Up @@ -82,3 +85,11 @@ func (mg *multiGetter) Get(key interface{}) (interface{}, error) {
}
return nil, firstErr
}

func (mg *multiGetter) GetAll() ([]interface{}, error) {
return nil, ErrUnimplemented
}

func (mg *multiGetter) PutHash(k interface{}, v interface{}) error {
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
return ErrUnimplemented
}
4 changes: 4 additions & 0 deletions pkg/cache/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ func (f getterFunc) Get(key interface{}) (interface{}, error) {
return f(key)
}

func (f getterFunc) GetAll() ([]interface{}, error) {
return nil, ErrUnimplemented
}

func TestMultiGetter(t *testing.T) {
value := "ok"
err := errors.New("err")
Expand Down
8 changes: 8 additions & 0 deletions pkg/cache/memorycache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,11 @@ func (c *Cache) Delete(key interface{}) error {
c.values.Delete(key)
return nil
}

func (c *Cache) GetAll() ([]interface{}, error) {
return nil, cache.ErrUnimplemented
}

func (c *Cache) PutHash(k interface{}, v interface{}) error {
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
return cache.ErrUnimplemented
}
8 changes: 8 additions & 0 deletions pkg/cache/memorycache/lru_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,11 @@ func (c *LRUCache) Delete(key interface{}) error {
c.cache.Remove(key)
return nil
}

func (c *LRUCache) GetAll() ([]interface{}, error) {
return nil, cache.ErrUnimplemented
}

func (c *LRUCache) PutHash(k interface{}, v interface{}) error {
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
return cache.ErrUnimplemented
}
8 changes: 8 additions & 0 deletions pkg/cache/memorycache/ttl_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,11 @@ func (c *TTLCache) Delete(key interface{}) error {
c.entries.Delete(key)
return nil
}

func (c *TTLCache) GetAll() ([]interface{}, error) {
return nil, cache.ErrUnimplemented
}

func (c *TTLCache) PutHash(k interface{}, v interface{}) error {
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
return cache.ErrUnimplemented
}
5 changes: 4 additions & 1 deletion pkg/cache/rediscache/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "go_default_library",
srcs = ["cache.go"],
srcs = [
"cache.go",
"hashcache.go",
],
importpath = "github.com/pipe-cd/pipe/pkg/cache/rediscache",
visibility = ["//visibility:public"],
deps = [
Expand Down
8 changes: 8 additions & 0 deletions pkg/cache/rediscache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,11 @@ func (c *RedisCache) Delete(k interface{}) error {
_, err := conn.Do("DEL", k)
return err
}

func (c *RedisCache) GetAll() ([]interface{}, error) {
return nil, cache.ErrUnimplemented
}

func (c *RedisCache) PutHash(k interface{}, v interface{}) error {
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
return cache.ErrUnimplemented
}
79 changes: 79 additions & 0 deletions pkg/cache/rediscache/hashcache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// Copyright 2021 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package rediscache

import (
"time"

redigo "github.com/gomodule/redigo/redis"

"github.com/pipe-cd/pipe/pkg/cache"
"github.com/pipe-cd/pipe/pkg/redis"
)

type RedisHashCache struct {
RedisCache
key string
}

func NewHashCache(redis redis.Redis, ttl time.Duration, key string) *RedisHashCache {
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
return &RedisHashCache{
RedisCache: RedisCache{
redis: redis,
},
key: key,
}
}

func NewTTLHashCache(redis redis.Redis, ttl time.Duration, key string) *RedisHashCache {
return &RedisHashCache{
RedisCache: RedisCache{
redis: redis,
ttl: uint(ttl.Seconds()),
},
key: key,
}
}

func (r *RedisHashCache) PutHash(k interface{}, v interface{}) error {
conn := r.redis.Get()
defer conn.Close()
_, err := conn.Do("HSET", r.key, k, v)
if r.ttl != 0 {
_, err = conn.Do("EXPIRE", r.key, r.ttl)
}
return err
}

func (r *RedisHashCache) GetAll() ([]interface{}, error) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who uses GetAll() currently? Any plan to use it later?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this PR is focusing on storeing

conn := r.redis.Get()
defer conn.Close()
reply, err := redigo.StringMap(conn.Do("HGETALL", r.key))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can not use StringMap in this case.
Because HGETALL is returning an array that containing both field and value.
So we have to use Array function to parse the reply. https://pkg.go.dev/github.com/gomodule/redigo/redis#Values

One more point is that instead of returning []interface{} maybe we should return map[interface{}]interface{}.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we adopt map[interface{}]interface{} as return value, then the current redigo.StringMap is good enough since its convert reply to map[string]string, how do you think about that 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it. Agree about that. 👍

@khanhtc1202 khanhtc1202 Jul 7, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adopted returning map[interface{}]interface{} idea on d7e89e0 🙆‍♀️
It looks much better now, thx 👍

if err != nil {
if err == redigo.ErrNil {
return nil, cache.ErrNotFound
}
return nil, err
}
if len(reply) == 0 {
return nil, cache.ErrNotFound
}

out := make([]interface{}, 0, len(reply))
for _, v := range reply {
out = append(out, v)
}
return out, nil
}