aws / aws-dax-go

AWS DAX SDK for the Go programming language. https://aws.amazon.com/dynamodb/dax
Apache License 2.0
47 stars 48 forks source link

NewConfigWithSession sets Config.ReadRetries to -1 #25

Closed aita closed 3 years ago

aita commented 4 years ago

When I initialize dax.Config with using NewConfigWIthSession like below, Config.ReadRetries becomes -1.

sess := session.Must(session.NewSession())
cfg := dax.NewConfigWithSession(*sess)
db := dax.New(cfg)

In this case, a dax client does not attempt any request without errors when I call GetItem or other methods.

ktseytlin commented 4 years ago

Based on what is written above, there appears to be two separate issues:

  1. Dax Client does not attempt any request without errors when calling GetItem

To try to reproduce this I used the Go TryDax Examples as a base, seen here: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/DAX.client.run-application-go.html

The code for this example is here: https://github.com/aws-samples/aws-dax-go-sample/blob/master/try_dax.go

I changed the daxClient function to the following:

func daxClient(endpoint, region string) (dynamodbiface.DynamoDBAPI, error) {
        sess := session.Must(session.NewSession())
        cfg := dax.NewConfigWithSession(*sess)
        cfg.HostPorts = []string{endpoint}
        cfg.Region = region
        return dax.New(cfg)
}

Then I ran the example again for GetItem, and it succeeded successfully. I cannot reproduce that the client does not attempt any request without errors.

Could you please add more information in order to reproduce your issue? I recommend double checking the following: security groups, if your host and Dax cluster are in the same VPC, if this works using a DefaultConfig, your credentials/profiles in the ~/.aws folder. Thanks.

  1. When initializing with NewConfigWithSession, then Config.ReadRetries is set to -1.

I was able to reproduce this issue in the same example as above. I changed the code the following:

func daxClient(endpoint, region string) (dynamodbiface.DynamoDBAPI, error) {
        sess := session.Must(session.NewSession())
        cfg := dax.NewConfigWithSession(*sess)
        cfg.HostPorts = []string{endpoint}
        cfg.Region = region
        fmt.Println(cfg.ReadRetries)
        return dax.New(cfg)
}

This printed -1 while running the example above. The defaultConfig for ReadRetries is 2, but when using NewConfigWithSession the default value for this either should be set manually or customized via MaxRetries.

Here are two ways to fix this:

func daxClient(endpoint, region string) (dynamodbiface.DynamoDBAPI, error) {
        sess := session.Must(session.NewSession())
        cfg := dax.NewConfigWithSession(*sess)
        cfg.HostPorts = []string{endpoint}
        cfg.Region = region
        cfg.ReadRetries = 2
        fmt.Println(cfg.ReadRetries)
        return dax.New(cfg)
}

func daxClient(endpoint, region string) (dynamodbiface.DynamoDBAPI, error) {
        sess := session.Must(session.NewSession(&aws.Config{
                MaxRetries: aws.Int(3),
        }))
        cfg := dax.NewConfigWithSession(*sess)
        cfg.HostPorts = []string{endpoint}
        cfg.Region = region
        fmt.Println(cfg.ReadRetries)
        return dax.New(cfg)
}
kevioke commented 4 years ago

@ktseytlin I'm currently facing a similar issue with using the dax.NewConfigWithSession API. Although in my case, I was trying to do BatchWriteItem instead of GetItem like @aita . The API returned successfully, but when I checked my table, nothing was updated or inserted which I expected to happen.

I believe this is a flaw with using dax.NewConfigWithSession and maybe even dax.NewWithSession. Both these function use the same logic to check for MaxRetries, but it fails to handle the magic value of -1 which signifies to use the service's default retry values https://github.com/aws/aws-sdk-go/blob/09f89e43e1bc1e647cd2bc83cf7cfb55c5be7fb3/aws/config.go#L14.

I haven't enabled full debug logging yet, but my assumption is that the requests are never sent at this point https://github.com/aws/aws-dax-go/blob/2528715f78d8b40d5e7477e648069abf19cf3f8b/dax/internal/client/single.go#L689

The core dynamodb package handles this correctly by checking for both nil and the magic value https://github.com/aws/aws-sdk-go/blob/09f89e43e1bc1e647cd2bc83cf7cfb55c5be7fb3/service/dynamodb/customizations.go#L32.

I see your point in explicitly setting the MaxRetries or ReadRetries and WriteRetries, but this is non-obvious for users of the dax library and perhaps not well documented. I propose we honor the special value and set the retries appropriately in the package so that it follows the rest of the aws-sdk-go conventions.

kevioke commented 4 years ago

Here's my proposed change https://github.com/aws/aws-dax-go/pull/26

ktseytlin commented 3 years ago

Got it, the motivation makes sense for me. Thanks for raising the fix.

mkadin commented 3 years ago

Until #26 is landed, can we update the name of this issue to "NewConfigWithSession constructs a client that always returns no results" or something that hints at the bug that this solves? We just lost a lot of time digging into this only to find it here right under our nose :)

kevinchristen commented 3 years ago

Fixed in v1.2.5.