chore: Add consul and dynamodb support #248

keelerm84 · 2024-10-22T19:57:26Z

No description provided.

kinyoklion · 2024-10-22T21:21:12Z

sdktests/server_side_persistence_consul.go

+	consul *consul.Client
+}
+
+// {{{ PersistentStore implementation


Is this a fold marker? Is it intentional?

Well yes and no. I was wanting to show where it started and ended, so I was putting in some easy markers that you can jump between.

Are they also default vim markers? Yes, but that's beside the point. 😄

Honestly, I was expecting more work in these files and figured it would be more useful than it is. So I will remove it.

kinyoklion · 2024-10-22T21:28:57Z

sdktests/server_side_persistence_dynamodb.go

+			RequestItems: map[string][]*dynamodb.WriteRequest{table: batch},
+		})
+		if err != nil {
+			// COVERAGE: can't simulate this condition in unit tests because we will only get this


Maybe this comment does apply, but the coverage suppression probably doesn't.

(Though I am not 100% sure it makes sense in its original location, which was also a test.)

cwaldren-ld · 2024-10-22T23:46:26Z

sdktests/server_side_persistence_base.go

+	"github.com/aws/aws-sdk-go/aws/credentials"
+	"github.com/aws/aws-sdk-go/aws/session"
+	"github.com/aws/aws-sdk-go/service/dynamodb"
+	c "github.com/hashicorp/consul/api"


nit: c -> consul

cwaldren-ld · 2024-10-22T23:47:22Z

sdktests/server_side_persistence_base.go

+				),
+		))
+
+		// Create a DynamoDB client from just a session.


what is the alternative?

cwaldren-ld · 2024-10-22T23:48:23Z

sdktests/server_side_persistence_consul.go

+
+// {{{ PersistentStore implementation
+
+func (c ConsulPersistentStore) DSN() string {


It's not necessarily bad to have this be a value receiver, but it does feel a bit confusing when the rest are pointer receivers. Go syntax sugar will make it work at the callsite.

It will make a copy of the ConsulPersistentStore struct, which could have unintended side-effects (like perhaps there is some mutex deep in the consul client that cannot be copied). Go is great.

I will update to be a pointer receiver.

cwaldren-ld · 2024-10-22T23:49:31Z

sdktests/server_side_persistence_base.go

-	Get(key string) (string, error)
-	GetMap(key string) (map[string]string, error)
-	WriteMap(key string, data map[string]string) error
+	Get(prefix, key string) (string, bool, error)


It's not immediately clear to me what (string, bool, error) means, but I suspect it's value, present, error?

Doc comment would work, or giving each return value a name would also be fine

Another option: since we hve the whole Opt thing going on, this could be an OptString, error

I like the optional string, so I'll go with that.

cwaldren-ld · 2024-10-22T23:55:50Z

sdktests/server_side_persistence_consul.go

+
+	if err != nil {
+		return nil, fmt.Errorf("list failed for %s: %s", key, err)
+	}


For errors returned from these wrapper methods, it could make sense to prefix them with the impl name so that it's clear exactly where the error is coming from. I don't have a good suggestion for the prefix though.

return nil, fmt.Errorf("consul-wrapper: foo bar")

ideally Consul itself is prefixing stuff with consul as well, but maybe not?

Because this can get repetitive, there are usually two options:

Make a helper like return nil, c.newErr(...)

Make a dedicated error type like return nil, &consulErr { action: "list", key: key, err: err } and have it render the error string

What you have is also probably fine. This is more of a best practice. Feel free to disregard.

I think I will leave this one to our refactoring work tomorrow.

cwaldren-ld · 2024-10-22T23:56:58Z

sdktests/server_side_persistence_consul.go

+func (c *ConsulPersistentStore) WriteMap(prefix, key string, data map[string]string) error {
+	kv := c.consul.KV()
+
+	// Start by reading the existing keys; we will later delete any of these that weren't in allData.


allData

think the comment needs to be tweaked

cwaldren-ld · 2024-10-23T00:01:13Z

sdktests/server_side_persistence_consul.go

+	if err != nil {
+		return fmt.Errorf("failed to get existing items prior to Init: %s", err)
+	}
+	oldKeys := make(map[string]bool)


If oldKeys is meant to represent a set, I find it clearer to use map[string]struct{}. That way, there's no need to check v (on line 81 of this diff).

On line 77, you'd delete from the set.

Yes, I thought about this when I was copying this work over from the actual implementations. I went back and forth on whether consistency was better.

I'll just fix it here.

sdktests/server_side_persistence_consul.go

cwaldren-ld · 2024-10-23T00:03:33Z

sdktests/server_side_persistence_consul.go

+}
+
+func batchOperations(kv *consul.KV, ops []*consul.KVTxnOp) error {
+	for i := 0; i < len(ops); {


I know this comes from the consul integration, but it would be good if we had a comment explaining what it is actually doing at a high level.

cwaldren-ld · 2024-10-23T00:05:45Z

sdktests/server_side_persistence_dynamodb.go

+
+const (
+	// Schema of the DynamoDB table
+	tablePartitionKey = "namespace"


Since these are all in the same package, you could access, say, tableName from the Redis file or whatever. It could be worth prefixing these to make them unique.

Better yet would be refactoring these into dedicated packages, that way they can't start accidentally sharing constants. But that would be a fair amount of work.

I'm surprised initedKey isn't being used in Redis impl (and is being used here)

Prefixed the dynamodb ones, and then moved the init one to the base file (I know that doesn't change anything, it's just an implication). Also changed its name and updated the usage in other places.

Co-authored-by: Casey Waldren <[email protected]>

cwaldren-ld · 2024-10-23T19:42:34Z

sdktests/server_side_persistence_base.go

 	"github.com/launchdarkly/go-sdk-common/v3/ldcontext"
 	"github.com/launchdarkly/go-sdk-common/v3/ldvalue"
 	"github.com/launchdarkly/go-server-sdk-evaluation/v3/ldbuilders"
 	"github.com/launchdarkly/sdk-test-harness/v2/framework/ldtest"
 	"github.com/launchdarkly/sdk-test-harness/v2/servicedef"
 )

+const (
+	PersistenceInitedKey = "$inited"


This is now a public constant accessible from other packages (capital P). Unclear if this is intended?

This reverts commit e398001.

keelerm84 added 2 commits October 17, 2024 13:29

chore: Add consul support

ba63c0d

chore: Add dynamodb support

f0c56f2

keelerm84 requested a review from a team as a code owner October 22, 2024 19:57

kinyoklion reviewed Oct 22, 2024

View reviewed changes

cwaldren-ld reviewed Oct 23, 2024

View reviewed changes

keelerm84 and others added 11 commits October 23, 2024 14:38

Apply suggestions from code review

323503a

Co-authored-by: Casey Waldren <[email protected]>

Change import name for consul

0dc128b

Remove old comment

9d02ebe

Replace tri-tuple with optional string and error

211b8c3

Remove markers

9c5fcbe

Pointers!

8f93168

Fix comment

a452f7c

Simplify set operations

bff9363

Doc on batchOperations

bbd0a24

Prefix constants

602b786

removed unused parameter and comment

a72f690

cwaldren-ld reviewed Oct 23, 2024

View reviewed changes

cwaldren-ld approved these changes Oct 23, 2024

View reviewed changes

keelerm84 added 4 commits October 24, 2024 10:30

Doesn't need to be exported

2866108

Bump golangci version

8f81650

Linting fixes

cff3b01

comments

f7e7427

cwaldren-ld approved these changes Oct 24, 2024

View reviewed changes

keelerm84 merged commit e398001 into feat/persistence-tests Oct 24, 2024
2 checks passed

keelerm84 deleted the mk/sdk-768/other-dbs branch October 24, 2024 16:46

keelerm84 added a commit that referenced this pull request Oct 24, 2024

Revert "chore: Add consul and dynamodb support (#248)"

b6fc49b

This reverts commit e398001.

keelerm84 added a commit that referenced this pull request Oct 31, 2024

chore: Add consul and dynamodb support (#248)

77ab32c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Add consul and dynamodb support #248

chore: Add consul and dynamodb support #248

keelerm84 commented Oct 22, 2024

kinyoklion Oct 22, 2024

keelerm84 Oct 23, 2024

kinyoklion Oct 22, 2024

cwaldren-ld Oct 22, 2024

cwaldren-ld Oct 22, 2024

cwaldren-ld Oct 22, 2024 •

edited

Loading

keelerm84 Oct 23, 2024

cwaldren-ld Oct 22, 2024

cwaldren-ld Oct 22, 2024

keelerm84 Oct 23, 2024

cwaldren-ld Oct 22, 2024

keelerm84 Oct 23, 2024

cwaldren-ld Oct 22, 2024

cwaldren-ld Oct 23, 2024

keelerm84 Oct 23, 2024

cwaldren-ld Oct 23, 2024

cwaldren-ld Oct 23, 2024

keelerm84 Oct 23, 2024

cwaldren-ld Oct 23, 2024


		// {{{ PersistentStore implementation

		func (c ConsulPersistentStore) DSN() string {

chore: Add consul and dynamodb support #248

chore: Add consul and dynamodb support #248

Conversation

keelerm84 commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwaldren-ld Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwaldren-ld Oct 22, 2024 •

edited

Loading