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.

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

Using DynamoDB as an option for persistence #101

Merged
merged 82 commits into from
Nov 15, 2021
Merged

Conversation

joshprzybyszewski
Copy link
Owner

@joshprzybyszewski joshprzybyszewski commented Aug 30, 2021

What broke / What you're adding

We have an in-memory DB, mysql DB, and mongodb DB. Unfornuately, in-memory is ephemeral, mysql is stood up in RDS and we pay for uptime, and I don't like mongodb. So let's add a DB that is pay-as-you-use and can store our games for us in aws.

How you did it

Add another set of services to talk to dynamodb using the AWS go sdk v2 (v1 is deprecated or something).
Also, stand up a local dynamodb instance in docker-compose so that you can test locally.

How to test it and how to try to break it

  1. Run make dockerbuild
  2. In the docker-compose, change the image of cribbage-server to image: cribbage:latest.
  3. go to localhost:18080 and start playing a game
  4. tear down the dynamodb container (using ctop) and notice that your games are all borked.

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #101 (22040cd) into master (a775d02) will increase coverage by 1.12%.
The diff coverage is 76.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
+ Coverage   69.35%   70.47%   +1.12%     
==========================================
  Files          80       86       +6     
  Lines        3455     3948     +493     
==========================================
+ Hits         2396     2782     +386     
- Misses        835      894      +59     
- Partials      224      272      +48     
Impacted Files Coverage Δ
server/setup.go 0.00% <0.00%> (ø)
...r/persistence/dynamo/dynamo_service_interaction.go 69.60% <69.60%> (ø)
server/persistence/dynamo/dynamo_service_player.go 71.07% <71.07%> (ø)
server/persistence/dynamo/dynamo_service_game.go 79.20% <79.20%> (ø)
server/persistence/dynamo/dynamodb.go 79.55% <79.55%> (ø)
server/persistence/dynamo/dynamo_utils.go 90.48% <90.48%> (ø)
server/dbutils.go 62.96% <100.00%> (+0.70%) ⬆️
server/interaction/npc.go 89.09% <100.00%> (+0.20%) ⬆️
server/persistence/dynamo/utils.go 100.00% <100.00%> (ø)
server/persistence/mysql/games.go 71.30% <0.00%> (+0.87%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a775d02...22040cd. Read the comment docs.

fmt.Println(dynamoResult)
for i, resp := range dynamoResult.Responses[dbName] {
dp := dynamoPlayer{}
// TODO unmarshalling a map doesn't work (i.e. the games)
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is busted, and it looks like they don't have plans to support in v1. Might as well hop in on v2...

aws/aws-sdk-go#3394 (comment)

@@ -31,5 +31,17 @@ func UnmarshalGame(b []byte) (model.Game, error) {
game.Actions[i] = pa
}

if game.Hands == nil {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Let's break this change out to a separate PR

Copy link
Owner Author

Choose a reason for hiding this comment

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

Comment on lines 42 to 47
func getConditionExpression(
pkType condExprType,
pk string,
skType condExprType,
sk string,
) *string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function does some disparate things depending on the inputs; an empty pk or sk is not required with notExists as compared to e.g. equalsID where you do need to provide those. This makes it a little tough to mentally parse calling sites. I wonder if it'd benefit from taking a struct as a single input arg instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

💡 I don't know if you'll like this idea since it's passing a lot of behavior around and it's kind of a lot of boilerplate -- but I kind of like this pattern as a way to provide composable options to a function

func getConditionExpression(opts ...conditionExpressionOption) *string {
    opts := conditionExpressionOptions{}
    for _, opt := range opts {
        opts = opt(opts)
    }
    // do whatever with your options
}

type conditionExpressionOptions {
    pkType condExprType
    pk string
    skType condExprType
    sk string
}

type conditionExpressionOption func(opts conditionExpressionOptions) conditionExpressionOptions

func pkNotExists(opts conditionExpressionOptions) conditionExpressionOptions {
    opts.pkType = notExists
    return opts
}

func skHasPrefix(pref string) conditionExpressionOption {
    return func(opts conditionExpressionOptions) conditionExpressionOptions {
        opts.skType = hasPrefix
        opts.sk = pref
        return opts
    }
}
// etc.

// calling sites
getConditionExpression(pkNotExists)
getConditionExpression(pkEquals(`def`), skHasPrefix(`abc`))

@cszczepaniak
Copy link
Collaborator

Finally finished my first pass

@cszczepaniak
Copy link
Collaborator

Just one outstanding discussion I'd like to finish up before I go for a QA

@joshprzybyszewski
Copy link
Owner Author

@cszczepaniak I think I've found a better way to express condition expressions that meets our needs without adding more complexity. let me know what you think.

Copy link
Collaborator

@cszczepaniak cszczepaniak left a comment

Choose a reason for hiding this comment

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

I will QA a little later, but the code looks great

@cszczepaniak
Copy link
Collaborator

QA +1
I was able to get the local docker stack up and running with dynamo
I was able to register two people, start a game, make some moves, then restart the go server and see the games still be there and in the correct state.
I killed dynamo and saw that I couldn't login or register a player. It seems we have a timeout of 30s for dynamo requests, so we may want to tweak that later, because the user sees it as an unresponsive webpage. Nonetheless, that's a follow up, and everything with dynamo seems peachy
image

@joshprzybyszewski joshprzybyszewski merged commit 978c0c6 into master Nov 15, 2021
@joshprzybyszewski joshprzybyszewski deleted the mongoToDynamo branch November 15, 2021 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants