From 2942c9275a185adfb40e28bc9da40472979cd44f Mon Sep 17 00:00:00 2001 From: Michael Coyne Date: Sat, 3 Feb 2024 10:06:07 -0500 Subject: [PATCH] more refactoring and add debug logging --- README.md | 1 - connections.go | 8 ++++---- counter.go | 8 +++++++- counter_test.go | 13 ++++++++++++- cycle.go | 29 ++++++++++++++++++----------- flag.go | 4 +++- kredis_test.go | 12 ++++++++++++ logging.go | 9 +++++---- proxy.go | 4 ++-- proxy_test.go | 3 +-- slot.go | 12 ++++++------ 11 files changed, 70 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index 5c1829f..68795fb 100644 --- a/README.md +++ b/README.md @@ -428,7 +428,6 @@ uniq.Clear() // DEL uniq ## TODO -- Finish remaining code `TODOs` - More test coverage: - Better coverage all possible generic types for collections - Test Redis commands with some sort of test env `ProcessHook`. This diff --git a/connections.go b/connections.go index 17db658..6d6f1d7 100644 --- a/connections.go +++ b/connections.go @@ -33,15 +33,15 @@ func SetConfiguration(name, namespace, url string, opts ...RedisOption) error { return nil } -func getConnection(name string) (*redis.Client, *string, error) { +func getConnection(name string) (*redis.Client, string, error) { config, configured := configs[name] if !configured { - return nil, nil, fmt.Errorf("%s is not a configured configuration", name) + return nil, "", fmt.Errorf("%s is not a configured configuration", name) } conn, ok := connections[name] if ok { - return conn, &config.namespace, nil + return conn, config.namespace, nil } conn = redis.NewClient(config.options) @@ -51,7 +51,7 @@ func getConnection(name string) (*redis.Client, *string, error) { conn.AddHook(newCmdLoggingHook(debugLogger)) } - return conn, &config.namespace, nil + return conn, config.namespace, nil } type cmdLoggingHook struct { diff --git a/counter.go b/counter.go index 1f58986..166f2e6 100644 --- a/counter.go +++ b/counter.go @@ -64,12 +64,18 @@ func (c *Counter) Decrement(by int64) (int64, error) { func (c *Counter) Value() (v int64) { v, err := c.client.Get(c.ctx, c.key).Int64() if err != nil && err != redis.Nil { - // TODO debug logging + if debugLogger != nil { + debugLogger.Warn("Counter#Value", err) + } } return } +func (c *Counter) ValueResult() (int64, error) { + return c.client.Get(c.ctx, c.key).Int64() +} + func (c *Counter) Reset() (err error) { _, err = c.client.Del(c.ctx, c.key).Result() return diff --git a/counter_test.go b/counter_test.go index 37eeb0b..a623e6b 100644 --- a/counter_test.go +++ b/counter_test.go @@ -1,6 +1,8 @@ package kredis -import "time" +import ( + "time" +) func (s *KredisTestSuite) TestCounter() { c, err := NewCounter("counter") @@ -58,3 +60,12 @@ func (s *KredisTestSuite) TestCounterWithDefaultAndExpiry() { s.Empty(c.Value()) } + +func (s *KredisTestSuite) TestCounterWIthBadConnection() { + c, _ := NewCounter("counter", WithConfigName("badconn")) + + s.Empty(c.Value()) + s.Equal([]string{ + "Counter#Value dial tcp [::1]:1234: connect: connection refused", + }, testWarnings) +} diff --git a/cycle.go b/cycle.go index 54068a5..b26010c 100644 --- a/cycle.go +++ b/cycle.go @@ -16,8 +16,19 @@ func NewCycle(key string, values []string, opts ...ProxyOption) (*Cycle, error) return &Cycle{Proxy: *proxy, values: values}, nil } -func (c *Cycle) Index() int64 { - return c.value() +func (c *Cycle) Index() (idx int64) { + idx, err := c.client.Get(c.ctx, c.key).Int64() + if err != nil && err != redis.Nil { + if debugLogger != nil { + debugLogger.Warn("Cycle#Index", err) + } + } + + return +} + +func (c *Cycle) IndexResult() (int64, error) { + return c.client.Get(c.ctx, c.key).Int64() } func (c *Cycle) Value() string { @@ -25,17 +36,13 @@ func (c *Cycle) Value() string { } func (c *Cycle) Next() (err error) { - value := (c.value() + 1) % int64(len(c.values)) - _, err = c.client.Set(c.ctx, c.key, value, c.expiresIn).Result() - - return -} - -func (c *Cycle) value() (v int64) { - v, err := c.client.Get(c.ctx, c.key).Int64() + idx, err := c.client.Get(c.ctx, c.key).Int64() if err != nil && err != redis.Nil { - // TODO debug logging + return // GET error } + value := (idx + 1) % int64(len(c.values)) + _, err = c.client.Set(c.ctx, c.key, value, c.expiresIn).Result() + return } diff --git a/flag.go b/flag.go index 8516ea8..55b6574 100644 --- a/flag.go +++ b/flag.go @@ -60,7 +60,9 @@ func (f *Flag) SoftMark(opts ...FlagMarkOption) error { func (f *Flag) IsMarked() bool { n, err := f.client.Exists(f.ctx, f.key).Result() if err != nil && err != redis.Nil { - // TODO debug logging + if debugLogger != nil { + debugLogger.Warn("Flag#IsMarked", err) + } } return n > 0 diff --git a/kredis_test.go b/kredis_test.go index 9282ebc..e138713 100644 --- a/kredis_test.go +++ b/kredis_test.go @@ -2,6 +2,7 @@ package kredis import ( "context" + "fmt" "testing" "github.com/stretchr/testify/suite" @@ -11,6 +12,14 @@ type KredisTestSuite struct { suite.Suite } +type testLogger struct{ stdLogger } + +var testWarnings []string + +func (tl testLogger) Warn(fnName string, err error) { + testWarnings = append(testWarnings, fmt.Sprintf("%s %s", fnName, err.Error())) +} + func (suite *KredisTestSuite) SetupTest() { // TODO use a unique namespace for each test (thus potentially enabling // parallel tests) @@ -18,6 +27,9 @@ func (suite *KredisTestSuite) SetupTest() { SetConfiguration("badconn", "", "redis://localhost:1234/0") EnableDebugLogging() + + testWarnings = []string{} + SetDebugLogger(&testLogger{}) } func (suite *KredisTestSuite) TearDownTest() { diff --git a/logging.go b/logging.go index 9fb9417..7a4a47c 100644 --- a/logging.go +++ b/logging.go @@ -30,8 +30,9 @@ func DisableDebugLogging() { debugLogger = nil } -// TODO add a way to configure a user provided value that implements the logging interface -// func SetCommandLogger(userLogger cmdLogging) +func SetDebugLogger(userLogger logging) { + debugLogger = userLogger +} func (log stdLogger) Info(cmd redis.Cmder, dur time.Duration) { name, key, args := cmdLogParts(cmd) @@ -45,8 +46,8 @@ func (log stdLogger) Info(cmd redis.Cmder, dur time.Duration) { } } -func (log stdLogger) Warn(msg string, err error) { - +func (log stdLogger) Warn(fnName string, err error) { + fmt.Printf("Kredis error in %s: %s", fnName, err.Error()) } var cmdColor = color.New(color.FgYellow).SprintFunc() diff --git a/proxy.go b/proxy.go index f938072..93f3726 100644 --- a/proxy.go +++ b/proxy.go @@ -26,8 +26,8 @@ func NewProxy(key string, opts ...ProxyOption) (*Proxy, error) { return nil, err } - if namespace != nil { - key = fmt.Sprintf("%s:%s", *namespace, key) + if namespace != "" { + key = fmt.Sprintf("%s:%s", namespace, key) } return &Proxy{ diff --git a/proxy_test.go b/proxy_test.go index 3b9575e..6d45771 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -14,8 +14,7 @@ func (s *KredisTestSuite) TestNewProxyWithConfigName() { s.NoError(e) s.NotNil(p) - - // TODO assert namespace and connnection details are right?? + s.Equal("key", p.key) } func (s *KredisTestSuite) TestNewProxyWithExpiresIn() { diff --git a/slot.go b/slot.go index c447bbd..f17a8de 100644 --- a/slot.go +++ b/slot.go @@ -1,8 +1,6 @@ package kredis import ( - "fmt" - "github.com/redis/go-redis/v9" ) @@ -93,8 +91,9 @@ func (s *Slot) TakenResult() (t int64, err error) { func (s *Slot) incr() { _, err := s.client.Incr(s.ctx, s.key).Result() if err != nil { - // TODO debug logging - fmt.Println(err) + if debugLogger != nil { + debugLogger.Warn("Slot#Reserve()", err) + } } s.RefreshTTL() @@ -103,8 +102,9 @@ func (s *Slot) incr() { func (s *Slot) decr() { _, err := s.client.Decr(s.ctx, s.key).Result() if err != nil { - // TODO debug logging - fmt.Println(err) + if debugLogger != nil { + debugLogger.Warn("Slot#Release()", err) + } } s.RefreshTTL()