Add DynamoDB support #2090
Add DynamoDB support #2090
Conversation
|
Also, this is for issue 2001 The reason why its one commit is because I didn't want to deal with all the merge conflicts and just made a new branch and copied over. |
aziz-shoko
force-pushed
the
aziz-shoko/dynamodb-clean
branch
from
f176e46 to
edfae48
Compare
|
Closed and Reopened because previous commit was trying to change 40 go.mod files, but now its good for review |
aziz-shoko
force-pushed
the
aziz-shoko/dynamodb-clean
branch
from
107874e to
f4d6891
Compare
|
@Umang01-hash Please run the pipeline again, I did all the typo and code quality fixes |
coolwednesday
left a comment
coolwednesday
left a comment
There was a problem hiding this comment.
Hey @aziz-shoko , I would need you to add documentation in docs folder under datasources. Refer to other documentation in datasources section. Also please add the docker container used in the contributing.md file.
You would also need to make changes in externaldb.go, container.go and mock container.go. Refer to this PR for datasource addition structure : #2076
Would also appreciate screenshots of running logs, traces, metrics. Would be easy to pinpoint any issues
| optFns ...func(*dynamodb.Options), | ||
| ) (*dynamodb.PutItemOutput, error) | ||
| GetItem( | ||
| ctx context.Context, |
There was a problem hiding this comment.
this should be *gofr.Context.
There was a problem hiding this comment.
Are you sure? I am using the function signatures from dynamodb package, switching to gofr.Context breaks it
There was a problem hiding this comment.
Hey no it won't break it .... gofr's context implements the context.Context interface. You will not face any issue.
|
HI @coolwednesday, thank you for the review. But I have a question, should I follow these function signatures? Right now the KVStore interface is only for basic key, value string inputs, but for dynamodb, I had a hashMap being passed in to support multiple types. Should I switch to key, value strings? |
If you check the value is actually marshaled and then inserted. you can have a hashMap also just handle the marshalling unmarshalling efficiently. |
|
I am gonna go ahead and close this PR, I had a fundamental misunderstanding of this framework and I don't think this is gonna work. Thank you for your time reviewing it 🙏 |
|
I will be going ahead and continuing the PR. |
|
Created #2242 . |
3 participants
Description:
I've added the DynamoDB kv-store and followed the same naming methods as NATS and Badger to stay consistent (Like Set, Get, and Delete operations). The code also largely follows the implementation details of NATS and Badger also for consistentcy and coherency. The only difference is that DynamoDB supports multiple attributes for keys so I made it accept a map of any values rather than just setting and getting strings.
I also ran my own integration tests against it using a local dynamodb container and even tested it against my own AWS account dynamodb. It worked as expected but I didn't commit the integration test file here because both NATS and Badger didn't have any specific implementation testing details. If needed, I can add the integration test file and the steps i took to test it.
Aside from that, all the requirements were met: