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.

Sign up for GitHub

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

Refactor NewAerospikeBackend function #164

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Refactor NewAerospikeBackend function #164

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9421e6
First commit
guscarreon Feb 23, 2024
63c5f7e
unit test description
guscarreon Feb 23, 2024
7eaa7ef
Fix comments
guscarreon Mar 13, 2024
accac66
Mock aerospike client instantiation
guscarreon Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 39 additions & 27 deletions backends/aerospike.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,34 +53,9 @@ type AerospikeBackend struct {

// NewAerospikeBackend validates config.Aerospike and returns an AerospikeBackend
func NewAerospikeBackend(cfg config.Aerospike, metrics *metrics.Metrics) *AerospikeBackend {
var hosts []*as.Host

clientPolicy := as.NewClientPolicy()
// cfg.User and cfg.Password are optional parameters
// if left blank in the config, they will default to the empty
// string and be ignored
clientPolicy.User = cfg.User
clientPolicy.Password = cfg.Password

// Aerospike's connection idle deadline default is 55 seconds. If greater than zero, this
// value will override
if cfg.ConnIdleTimeoutSecs > 0 {
clientPolicy.IdleTimeout = time.Duration(cfg.ConnIdleTimeoutSecs) * time.Second
}

// Aerospike's default connection queue size per node is 256.
// If cfg.ConnQueueSize is greater than zero, it will override the default.
if cfg.ConnQueueSize > 0 {
clientPolicy.ConnectionQueueSize = cfg.ConnQueueSize
}

if len(cfg.Host) > 1 {
hosts = append(hosts, as.NewHost(cfg.Host, cfg.Port))
log.Info("config.backend.aerospike.host is being deprecated in favor of config.backend.aerospike.hosts")
}
for _, host := range cfg.Hosts {
hosts = append(hosts, as.NewHost(host, cfg.Port))
}
clientPolicy := generateAerospikeClientPolicy(cfg)
hosts := generateHostsList(cfg)

client, err := as.NewClientWithPolicyAndHost(clientPolicy, hosts...)
if err != nil {
Expand Down Expand Up @@ -110,6 +85,43 @@ func NewAerospikeBackend(cfg config.Aerospike, metrics *metrics.Metrics) *Aerosp
}
}

// generateAerospikeClientPolicy returns an Aerospike ClientPolicy object configured according to values
// in config.Aerospike fields
func generateAerospikeClientPolicy(cfg config.Aerospike) *as.ClientPolicy {
clientPolicy := as.NewClientPolicy()
// cfg.User and cfg.Password are optional parameters
// if left blank in the config, they will default to the empty
// string and be ignored
clientPolicy.User = cfg.User
clientPolicy.Password = cfg.Password

// Connection idle timeout default is 55 seconds
if cfg.ConnIdleTimeoutSecs > 0 {
clientPolicy.IdleTimeout = time.Duration(cfg.ConnIdleTimeoutSecs) * time.Second
}

// Default connection queue size per node is 256
if cfg.ConnQueueSize > 0 {
Copy link

Choose a reason for hiding this comment

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

I don't think this comment is necessary, the well-written code explains it I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Probably a leftover. I sometimes structure logic into comments before writing the actual code. Removed.

clientPolicy.ConnectionQueueSize = cfg.ConnQueueSize
}

return clientPolicy
}

func generateHostsList(cfg config.Aerospike) []*as.Host {
var hosts []*as.Host

if len(cfg.Host) > 1 {
hosts = append(hosts, as.NewHost(cfg.Host, cfg.Port))
log.Info("config.backend.aerospike.host is being deprecated in favor of config.backend.aerospike.hosts")
}
for _, host := range cfg.Hosts {
hosts = append(hosts, as.NewHost(host, cfg.Port))
}

return hosts
}

// Get creates an aerospike key based on the UUID key parameter, perfomrs the client's Get call
// and validates results. Can return a KEY_NOT_FOUND error or other Aerospike server errors
func (a *AerospikeBackend) Get(ctx context.Context, key string) (string, error) {
Expand Down
212 changes: 185 additions & 27 deletions backends/aerospike_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"testing"
"time"

as "github.com/aerospike/aerospike-client-go/v6"
as_types "github.com/aerospike/aerospike-client-go/v6/types"
Expand All @@ -17,70 +18,228 @@ import (
"github.com/stretchr/testify/assert"
)

func TestNewAerospikeBackend(t *testing.T) {
func TestGenerateAerospikeClientPolicy(t *testing.T) {
testCases := []struct {
desc string
inCfg config.Aerospike
expected *as.ClientPolicy
}{
{
desc: "Blank configuration",
inCfg: config.Aerospike{},
expected: as.NewClientPolicy(),
},
{
desc: "Config with credentials",
inCfg: config.Aerospike{
User: "foobar",
Password: "password",
},
expected: &as.ClientPolicy{
User: "foobar",
Password: "password",
AuthMode: as.AuthModeInternal,
Timeout: 30 * time.Second,
IdleTimeout: 0 * time.Second,
LoginTimeout: 10 * time.Second,
ConnectionQueueSize: 100,
OpeningConnectionThreshold: 0,
FailIfNotConnected: true,
TendInterval: time.Second,
LimitConnectionsToQueueSize: true,
IgnoreOtherSubnetAliases: false,
MaxErrorRate: 100,
ErrorRateWindow: 1,
},
},
{
desc: "Config with ConnIdleTimeoutSecs",
inCfg: config.Aerospike{
ConnIdleTimeoutSecs: 3600,
},
expected: &as.ClientPolicy{
AuthMode: as.AuthModeInternal,
Timeout: 30 * time.Second,
IdleTimeout: 3600 * time.Second,
LoginTimeout: 10 * time.Second,
ConnectionQueueSize: 100,
OpeningConnectionThreshold: 0,
FailIfNotConnected: true,
TendInterval: time.Second,
LimitConnectionsToQueueSize: true,
IgnoreOtherSubnetAliases: false,
MaxErrorRate: 100,
ErrorRateWindow: 1,
},
},
{
desc: "Config with ConnIdleTimeoutSecs",
inCfg: config.Aerospike{
ConnQueueSize: 31416,
},
expected: &as.ClientPolicy{
AuthMode: as.AuthModeInternal,
Timeout: 30 * time.Second,
IdleTimeout: 0 * time.Second,
LoginTimeout: 10 * time.Second,
ConnectionQueueSize: 31416,
OpeningConnectionThreshold: 0,
FailIfNotConnected: true,
TendInterval: time.Second,
LimitConnectionsToQueueSize: true,
IgnoreOtherSubnetAliases: false,
MaxErrorRate: 100,
ErrorRateWindow: 1,
},
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
asPolicy := generateAerospikeClientPolicy(tc.inCfg)
assert.Equal(t, tc.expected, asPolicy)
})
}
}

func TestGenerateHostsList(t *testing.T) {
type logEntry struct {
msg string
lvl logrus.Level
}

testCases := []struct {
desc string
inCfg config.Aerospike
expectPanic bool
expectedOutput []*as.Host
expectedLogEntries []logEntry
}{
{
desc: "Unable to connect hosts fakeTestUrl panic and log fatal error when passed additional hosts",
desc: "No host, hosts nor port",
inCfg: config.Aerospike{},
expectedOutput: []*as.Host{},
},
{
desc: "Hosts list but no host nor port",
inCfg: config.Aerospike{
Hosts: []string{"foo.com", "bat.com"},
Port: 8888,
Hosts: []string{"foo.com", "bar.com"},
},
expectedOutput: []*as.Host{
as.NewHost("foo.com", 0),
as.NewHost("bar.com", 0),
},
expectPanic: true,
},
{
desc: "Host no port nor hosts list",
inCfg: config.Aerospike{Host: "foo.com"},
expectedOutput: []*as.Host{as.NewHost("foo.com", 0)},
expectedLogEntries: []logEntry{
{
msg: "Error creating Aerospike backend: ResultCode: TIMEOUT, Iteration: 0, InDoubt: false, Node: <nil>: command execution timed out on client: See `Policy.Timeout`",
lvl: logrus.FatalLevel,
msg: "config.backend.aerospike.host is being deprecated in favor of config.backend.aerospike.hosts",
lvl: logrus.InfoLevel,
},
},
},
{
desc: "Unable to connect host and hosts panic and log fatal error when passed additional hosts",
desc: "Host and hosts list, no port",
inCfg: config.Aerospike{
Host: "fakeTestUrl.foo",
Hosts: []string{"foo.com", "bat.com"},
Port: 8888,
Host: "foo.com",
Hosts: []string{"foo.com", "bar.com"},
},
expectedOutput: []*as.Host{
as.NewHost("foo.com", 0),
as.NewHost("foo.com", 0),
as.NewHost("bar.com", 0),
},
expectPanic: true,
expectedLogEntries: []logEntry{
{
msg: "config.backend.aerospike.host is being deprecated in favor of config.backend.aerospike.hosts",
lvl: logrus.InfoLevel,
},
{
msg: "Error creating Aerospike backend: ResultCode: TIMEOUT, Iteration: 0, InDoubt: false, Node: <nil>: command execution timed out on client: See `Policy.Timeout`",
lvl: logrus.FatalLevel,
},
},
},
{
desc: "Unable to connect host panic and log fatal error",
desc: "Port but no host nor hosts list",
inCfg: config.Aerospike{Port: 8888},
expectedOutput: []*as.Host{},
},
{
desc: "Port, hosts list, no host",
inCfg: config.Aerospike{
Port: 8888,
Hosts: []string{"foo.com", "bar.com"},
},
expectedOutput: []*as.Host{
as.NewHost("foo.com", 8888),
as.NewHost("bar.com", 8888),
},
},
{
desc: "Port and host but no hosts list",
inCfg: config.Aerospike{
Host: "fakeTestUrl.foo",
Port: 8888,
Host: "foo.com",
},
expectPanic: true,
expectedOutput: []*as.Host{as.NewHost("foo.com", 8888)},
expectedLogEntries: []logEntry{
{
msg: "config.backend.aerospike.host is being deprecated in favor of config.backend.aerospike.hosts",
lvl: logrus.InfoLevel,
},
},
},
{
desc: "Port, host and hosts list",
inCfg: config.Aerospike{
Port: 8888,
Host: "foo.com",
Hosts: []string{"foo.com", "bar.com"},
},
expectedOutput: []*as.Host{
as.NewHost("foo.com", 8888),
as.NewHost("foo.com", 8888),
as.NewHost("bar.com", 8888),
},
expectedLogEntries: []logEntry{
{
msg: "Error creating Aerospike backend: ResultCode: TIMEOUT, Iteration: 0, InDoubt: false, Node: <nil>: command execution timed out on client: See `Policy.Timeout`",
lvl: logrus.FatalLevel,
msg: "config.backend.aerospike.host is being deprecated in favor of config.backend.aerospike.hosts",
lvl: logrus.InfoLevel,
},
},
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
asHosts := generateHostsList(tc.inCfg)
assert.ElementsMatch(t, tc.expectedOutput, asHosts)
})
}
}

func TestNewAerospikeBackend(t *testing.T) {
testCases := []struct {
desc string
inCfg config.Aerospike
expectedLogEntryLevels []logrus.Level
}{
{
// Unable to connect to host in Hosts list. Expect fatal level log entry
desc: "Hosts_slice_and_port",
inCfg: config.Aerospike{
Hosts: []string{"fakeUrl"},
Port: 8888,
},
expectedLogEntryLevels: []logrus.Level{logrus.FatalLevel},
},
{
// Unable to connect to URL in Host field. Expect fatal level log entry and
// an info level log entry because the Host string is deprecated
desc: "Host_string_and_port",
inCfg: config.Aerospike{
Host: "fakeUrl",
Port: 8888,
},
expectedLogEntryLevels: []logrus.Level{logrus.InfoLevel, logrus.FatalLevel},
},
}

// logrus entries will be recorded to this `hook` object so we can compare and assert them
hook := test.NewGlobal()
Expand All @@ -92,10 +251,9 @@ func TestNewAerospikeBackend(t *testing.T) {
for _, test := range testCases {
// Run test
assert.Panics(t, func() { NewAerospikeBackend(test.inCfg, nil) }, "Aerospike library's NewClientWithPolicyAndHost() should have thrown an error and didn't, hence the panic didn't happen")
if assert.Len(t, hook.Entries, len(test.expectedLogEntries), test.desc) {
for i := 0; i < len(test.expectedLogEntries); i++ {
assert.Equal(t, test.expectedLogEntries[i].msg, hook.Entries[i].Message, test.desc)
assert.Equal(t, test.expectedLogEntries[i].lvl, hook.Entries[i].Level, test.desc)
if assert.Len(t, hook.Entries, len(test.expectedLogEntryLevels), test.desc) {
for i := 0; i < len(test.expectedLogEntryLevels); i++ {
assert.Equal(t, test.expectedLogEntryLevels[i], hook.Entries[i].Level, test.desc)
}
}

Expand Down
Loading