use aws-sdk-go-v2 #43

shamaton · 2023-05-15T14:28:32Z

Issue #, if available:

#2

Description of changes:

use aws-sdk-go-v2 in the entire code.
If you have any questions, please feel free to ask.

Summary of the changes

Although there are numerous changes in the code, I have made the following modifications:

Import aws-sdk-go-v2, but no import aws-sdk-go
All error is wrapped by using aws/smithy-go
Compatible dynamodb V2 API (see also: dax.DynamoDBAPI)
Functions such as Retryer use aws-sdk-go-v2.
For functionalities that are difficult to maintain compatibility with, removed them temporarily (e.g., PutItemRequest, BatchGetItemPages)
Of course all test passed

Verification

I have created a validation repository to confirm if the changed code is actually usable.
https://github.com/shamaton/aws-dax-go-v2-test

API: GetItem
API: PutItem (also case: ConditionalExpression)
API: UpdateItem
API: DeleteItem
API: Query
API: Scan (also Count)
API: BatchGetItems
API: BatchWriteItems
API: TransactGetItems
Checked if the DAX cache is enable

The future of this pull request

I think there are the following options:

Merge this pull request into the master branch.
Merge the changes of this pull request into another branch, such as v2, and proceed with the operation.
Create a separate repository to make it usable, for example, aws/aws-dax-go-v2.
Reject this pull request (unfortunate decision).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

shamaton · 2023-07-11T13:13:21Z

Any comments or reviews from aws staff?

anudeeb · 2023-07-14T06:34:42Z

@shamaton Thank you for raising the PR. We are currently reviewing it and will get back to you soon.

anudeeb

I reviewed few files and added some comments. Please let us know if you have further questions.

anudeeb · 2023-07-27T10:41:48Z

dax/api.go

+	o := d.config.requestOptions(false, opts...)
+	ctx, cancel := d.ContextWithTimeout(ctx)
+	defer cancel()
+	return d.client.PutItemWithOptions(ctx, input, o)


I see these 4 lines are similar in all the methods. Is it possible to avoid the duplicate code?

We can avoid duplicate code by writing code like this:

func (d *Dax) TransactGetItems(ctx context.Context, input *dynamodb.TransactGetItemsInput, opts ...func(*dynamodb.Options)) (*dynamodb.TransactGetItemsOutput, error) { return f(d, ctx, input, d.client.TransactGetItemsWithOptions, opts...) } func f[T any, U any](d *Dax, ctx context.Context, input T, outputFn func (context.Context, T, client.RequestOptions) (U, error), opts ...func(options *dynamodb.Options)) (U, error) { o := d.config.requestOptions(true, opts...) ctx, cancel := d.ContextWithTimeout(ctx) defer cancel() return outputFn(ctx, input, o) }

But it requires generics, go version must be 1.18 or higher. With the current supported version, I think any further changes would add overhead.

anudeeb · 2023-07-27T10:45:51Z

dax/api.go

+//func (d *Dax) BatchGetItemPages(ctx context.Context, input *dynamodb.BatchGetItemInput, fn func(*dynamodb.BatchGetItemOutput, bool) bool, opts ...func(*dynamodb.Options)) error {
+//	p := request.Pagination{
+//		NewRequest: func() (*request.Request, error) {
+//			var inCpy *dynamodb.BatchGetItemInput
+//			if input != nil {
+//				tmp := *input
+//				inCpy = &tmp
+//			}
+//			req, _ := d.BatchGetItemRequest(inCpy)
+//			req.SetContext(ctx)
+//			req.ApplyOptions(opts...)
+//			return req, nil
+//		},
+//	}
+//
+//	for p.Next() {
+//		if !fn(p.Page().(*dynamodb.BatchGetItemOutput), !p.HasNextPage()) {
+//			break
+//		}
+//	}
+//
+//	return p.Err()
+//}


We can probably remove these lines.

When we get a similar paginator from DynamoDB, we can add it similarly for DAX - aws/aws-sdk-go-v2#1707

delete in 39275d2

anudeeb · 2023-07-27T11:00:41Z

dax/api.go

+	if d.config.RequestTimeout > 0 {
+		return context.WithTimeout(ctx, d.config.RequestTimeout)
+	}


This would mean that we would never honour timeout set in ctx right? We would only honour the timeout that's set in d.config.RequestTimeout.

For v2, we are thinking to eliminate d.config.RequestTimeout and just rely on the timeout set in ctx. What are your thoughts on this?

Please refer to #46 (comment) for discussion around this topic.

No, this code will always set d.config.RequestTimeout as timeout in context. A timeout in the parent context is also valid.
https://go.dev/play/p/NNgqWXaEbUH

If we want to remove d.config.RequestTimeout, we can do so but setting timeout is completely up to the user.
I do not recommend it. We should always have timeout set to the default value recommended by aws unless set by user.

This issue #46 (comment) is that the RequestTimeout is not set unless the context is nil right?

So what we are saying here is that both values will be read but if the user passes a ctx with a timeout, this will override config.RequestTimeout? That sounds fine by me...

Please document this! Above this code

aws-dax-go/dax/service.go

Line 49 in b128174

RequestTimeout time.Duration

@anudeeb
I thought about this again.

It may be better to let user set context.WithTimeout. This is because the amount of time it takes depends on kinds of the data processing. For example, fetching or counting lots of data may be different than fetching single record.

so I think it is good thoughts to eliminate config.RequestTimeout

changed in eaf620e

miparnisari · 2023-07-31T20:35:23Z

Question: since there is no way of testing DAX locally, has this PR been tested against an actual DAX cluster?

miparnisari · 2023-07-31T21:50:33Z

dax/internal/client/cluster.go

-			if err != nil && opt.Logger != nil && opt.LogLevel.Matches(aws.LogDebugWithRequestRetries) {
-				opt.Logger.Log(fmt.Sprintf("DEBUG: Error in executing request %s/%s. : %s", service, op, err))
-			}
+		if opt.Logger != nil {


here and in other places, why do you need this check? set a default NoOp logger :)

Logger can be nil if you created a CustomConfig without using a DefaultConfig.
It may be good to set Nop when Logger is nil.

shamaton · 2023-08-01T14:48:00Z

@miparnisari

Question: since there is no way of testing DAX locally, has this PR been tested against an actual DAX cluster?

Yes. I created a validation repository to confirm if the changed code is actually usable.
https://github.com/shamaton/aws-dax-go-v2-test

Please, see verfication part of summary.

bsandeepan · 2023-08-09T06:25:49Z

Hi, if this PR gets approved, by when can we expect these changes ready for production usage?

shamaton · 2023-09-30T04:41:13Z

Hi, is status reviewing now?

srri · 2023-10-30T21:46:39Z

Bumping interest on this PR. Lots of us are waiting for a SDK v2 version of this DAX client. Feels like it's been abandoned..

ngjmcdonald · 2023-12-22T15:10:38Z

Adding another voice, hoping to get this SDK upgraded to the new version in the new year! Echoing the previous comment it feels like it's been abandoned but DAX is still encouraged as the cache for DynamoDB

Joao-Pedro-MB · 2024-01-26T00:26:08Z

Hello everyone. Any updates on this PR? I have been studying the Shamaton changes, and they are quite solid. I was able to make it work very easily, even with a newer Go version in go.mod.

shamaton added 30 commits April 22, 2023 21:30

go get github.com/aws/aws-sdk-go-v2/service/dynamodb

ac0984e

import github.com/aws/aws-sdk-go-v2/service/dynamodb

67df3b2

fix dax/internal/client/projection.go

9bd6e90

fix translate methods @ request.go

45cf488

fix expressions

0ecfa56

use aws-sdk-go-v2/service/dynamodb @ request.go

02e0852

use aws-sdk-go-v2/service/dynamodb @ attrval.go

aba99e9

use aws-sdk-go-v2/service/dynamodb @ item.go

9dcab3b

use aws-sdk-go-v2/service/dynamodb @ response.go

7f20648

use aws-sdk-go-v2/service/dynamodb @ projection.go

4f55f2a

use aws-sdk-go-v2/service/dynamodb @ single.go

f8a8bf9

add github.com/aws/aws-sdk-go-v2

8be6663

fix internal/cbor tests

4879ea2

fix internal/parser tests

cb867a9

fix api_test, pagination_test

39f8355

use smithy error in cbor

e848698

add client logger to replace aws.Logger

65beca5

use client.Logger

61804f1

dax.Dax is compatible to v2 dynamodb

1be9c6e

use v2 credential provider

1cec2eb

fix client tests

a30a414

update client package

d303535

use smithy-go logging

3e8e422

fix config.HostPorts

285c129

fix service_test

e2397b6

WithSession functions replace to WithSDKConfig

46730e4

fix logger called method

8172ddf

delete NewDaxRequest

b5a67d6

use v2 aws.Retryer

d75a83a

use context.Context

5190eb4

shamaton added 10 commits June 1, 2023 23:46

update go.mod, not required github.com/aws/aws-sdk-go

8ac48ad

fix legacy.go

9ff4a05

fix projection codes and tests

8688bf1

fix error_test.go

20de45b

CreateGlobalTableRequest is not available in dynamodb

d8872c6

implement paging functionality if necessary

0df8dc4

if ProjectionExpression is not nil, Should set Select type

5d1e5d7

delete config validation and request_options tests

9a96dfb

delete dax original retryer

99cf40a

delete unused code in convertDaxError

5cc083a

shamaton force-pushed the v2 branch from 4937cd6 to 5cc083a Compare June 9, 2023 14:46

shamaton changed the title ~~[WIP] use aws-sdk-go-v2~~ use aws-sdk-go-v2 Jun 10, 2023

taiyow mentioned this pull request Jun 30, 2023

aws-sdk-go-v2 support #2

Open

anudeeb requested changes Jul 27, 2023

View reviewed changes

miparnisari reviewed Jul 31, 2023

View reviewed changes

delete masked BatchGetItemPages function

39275d2

Merge branch 'master' into v2

9154e67

shamaton requested a review from anudeeb August 9, 2023 03:31

eliminate Config.RequestTimeout

eaf620e

miparnisari mentioned this pull request Feb 9, 2024

Cannot create DAX client with session - InvalidParameter: custom handlers not supported #17

Closed

miparnisari mentioned this pull request May 11, 2024

MIGRATION ISSUE: DAX not supported aws/aws-sdk#743

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use aws-sdk-go-v2 #43

use aws-sdk-go-v2 #43

shamaton commented May 15, 2023 •

edited

shamaton commented Jul 11, 2023

anudeeb commented Jul 14, 2023

anudeeb left a comment

anudeeb Jul 27, 2023

shamaton Aug 1, 2023

anudeeb Jul 27, 2023

shamaton Aug 1, 2023

anudeeb Jul 27, 2023

shamaton Aug 3, 2023 •

edited

miparnisari Aug 4, 2023 •

edited

shamaton Aug 10, 2023 •

edited

shamaton Sep 16, 2023

miparnisari commented Jul 31, 2023

miparnisari Jul 31, 2023

shamaton Aug 3, 2023

shamaton commented Aug 1, 2023

bsandeepan commented Aug 9, 2023

shamaton commented Sep 30, 2023

srri commented Oct 30, 2023

ngjmcdonald commented Dec 22, 2023

Joao-Pedro-MB commented Jan 26, 2024

use aws-sdk-go-v2 #43

Are you sure you want to change the base?

use aws-sdk-go-v2 #43

Conversation

shamaton commented May 15, 2023 • edited

Summary of the changes

Verification

The future of this pull request

shamaton commented Jul 11, 2023

anudeeb commented Jul 14, 2023

anudeeb left a 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

shamaton Aug 3, 2023 • edited

Choose a reason for hiding this comment

miparnisari Aug 4, 2023 • edited

Choose a reason for hiding this comment

shamaton Aug 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miparnisari commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamaton commented Aug 1, 2023

bsandeepan commented Aug 9, 2023

shamaton commented Sep 30, 2023

srri commented Oct 30, 2023

ngjmcdonald commented Dec 22, 2023

Joao-Pedro-MB commented Jan 26, 2024

shamaton commented May 15, 2023 •

edited

shamaton Aug 3, 2023 •

edited

miparnisari Aug 4, 2023 •

edited

shamaton Aug 10, 2023 •

edited