Updating aws sdk to v3 #116
Conversation
bilindhajer
left a comment
bilindhajer
left a comment
There was a problem hiding this comment.
This repo is callback hell 😅 . I left some questions
| @@ -1,11 +1,12 @@ | |||
| var AWS = require('aws-sdk'); | |||
| var { S3Client } = require('@aws-sdk/client-s3'); | |||
| var { Upload } = require('@aws-sdk/lib-storage'); | |||
There was a problem hiding this comment.
I know you didnt introduce this pattern but we should be using const and not var for this
|
|
||
| upload.on('httpUploadProgress', function(progress) { | ||
| log('[segment %s] Uploaded %s bytes', index, progress.loaded); | ||
| size = progress.total; |
There was a problem hiding this comment.
intresting it seems progress.loaded was available before this change, assuming total is not deprecated. Do you know the difference between the two?
There was a problem hiding this comment.
In @aws-sdk/lib-storage's Upload class the httpUploadProgress event provides a progress object, but progress.total can be undefined or not reliably set.
I'm not married to this change but when testing I noticed the console spitting out 'undefined' for this value. I went down a deep rabbit hole and found this gh issue which recommended using the loaded over total
| }); | ||
| cwClient.send(new PutMetricDataCommand(params)) | ||
| .then(() => { | ||
| // Success |
| console.log('--------------------------------'); | ||
| } | ||
| }); | ||
| s3Client.send(new GetObjectCommand(s3url)) |
There was a problem hiding this comment.
way too many callbacks being used here not related to your PR but it makes everything harder to read, maybe we can come back and use the await/async model
| }, | ||
| "homepage": "https://github.com/mapbox/dynamodb-replicator", | ||
| "dependencies": { | ||
| "@aws-sdk/client-cloudwatch": "^3.445.0", |
There was a problem hiding this comment.
When you do npm ls aws-sdk do you still see it anywhere? Not saying that if you did it would block this PR but I don't think we are done until we remove it at all levels
bilindhajer
left a comment
bilindhajer
left a comment
There was a problem hiding this comment.
My suggestions are not blocking but I do think this repo needs a big refactor/modernization
2 participants
What Changed?
Testing
Current Setup
In coredb-staging you can place an item.
I'll place a test item here in eu-west-1 (primary db table)
From there I can query and validate it exists in eu-west-1
Then, the coredb replicator runs and puts that record in us-west-2. I perform the same query in us-west-2 and observe:
So this works as a replicator, since global tables is not on.
Testing with new changes
In this PR https://github.com/mapbox/coredb-replicator/pull/77 I upgrade the dynamodb replicator to use its beta state. This has been deployed to staging. Let's walk through the same workflow to make sure coredb-staging can handle the testing.
Create a new entry in eu-west-1:
From there we can query to see if this has been replicated in us-west-2:
As you can see, this was replicated without any issue. The lambda sustained no errors in the process. This package is working as expected.