-
Notifications
You must be signed in to change notification settings - Fork 46
Updating aws sdk to v3 #116
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| # [11.0.1](https://github.com/mapbox/dynamodb-replicator/pull/116) July 21, 2025 | ||
|
|
||
| **Potential Breaking Change**: Upgrading all aws sdk v2 | ||
| usage to aws sdk v3. AWS sdk v2 EOL is September 2025, so | ||
| this change is mandatory for standard upkeep. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| var AWS = require('aws-sdk'); | ||
| var { S3Client } = require('@aws-sdk/client-s3'); | ||
| var { Upload } = require('@aws-sdk/lib-storage'); | ||
| var Dyno = require('@mapbox/dyno'); | ||
| var stream = require('stream'); | ||
| var zlib = require('zlib'); | ||
|
|
||
| module.exports = function(config, done) { | ||
| var primary = Dyno(config); | ||
| var s3 = new AWS.S3(); | ||
| var s3Client = new S3Client({ region: config.region }); | ||
|
|
||
| var log = config.log || console.log; | ||
|
|
||
|
|
@@ -40,20 +41,30 @@ module.exports = function(config, done) { | |
|
|
||
| log('[segment %s] Starting backup job %s of %s', index, config.backup.jobid, config.region + '/' + config.table); | ||
|
|
||
| s3.upload({ | ||
| Bucket: config.backup.bucket, | ||
| Key: key, | ||
| Body: data | ||
| }, function(err) { | ||
| if (err) return next(err); | ||
| log('[segment %s] Uploaded dynamo backup to s3://%s/%s', index, config.backup.bucket, key); | ||
| log('[segment %s] Wrote %s items to backup', index, count); | ||
| next(); | ||
| }).on('httpUploadProgress', function(progress) { | ||
| const upload = new Upload({ | ||
| client: s3Client, | ||
| params: { | ||
| Bucket: config.backup.bucket, | ||
| Key: key, | ||
| Body: data | ||
| } | ||
| }); | ||
|
|
||
| upload.on('httpUploadProgress', function(progress) { | ||
| log('[segment %s] Uploaded %s bytes', index, progress.loaded); | ||
| size = progress.total; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. intresting it seems progress.loaded was available before this change, assuming total is not deprecated. Do you know the difference between the two?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| size = progress.loaded; | ||
| }); | ||
|
|
||
| upload.done() | ||
| .then(() => { | ||
| log('[segment %s] Uploaded dynamo backup to s3://%s/%s', index, config.backup.bucket, key); | ||
| log('[segment %s] Wrote %s items to backup', index, count); | ||
| next(); | ||
| }) | ||
| .catch(err => { | ||
| next(err); | ||
| }); | ||
|
|
||
| function next(err) { | ||
| if (err) return done(err); | ||
| done(null, { size: size, count: count }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ var fastlog = require('../fastlog'); | |
| var args = require('minimist')(process.argv.slice(2)); | ||
| var crypto = require('crypto'); | ||
| var s3urls = require('s3urls'); | ||
| var AWS = require('aws-sdk'); | ||
| var { CloudWatchClient, PutMetricDataCommand } = require('@aws-sdk/client-cloudwatch'); | ||
|
|
||
| function usage() { | ||
| console.error(''); | ||
|
|
@@ -64,7 +64,7 @@ backup(config, function(err, details) { | |
| if (err) log.error(err); | ||
|
|
||
| if (args.metric) { | ||
| var cw = new AWS.CloudWatch({ region: config.region }); | ||
| var cwClient = new CloudWatchClient({ region: config.region }); | ||
| var params = { | ||
| Namespace: args.metric, | ||
| MetricData: [] | ||
|
|
@@ -107,8 +107,12 @@ backup(config, function(err, details) { | |
| }); | ||
| } | ||
|
|
||
| cw.putMetricData(params, function(err) { | ||
| if (err) log.error(err); | ||
| }); | ||
| cwClient.send(new PutMetricDataCommand(params)) | ||
| .then(() => { | ||
| // Success | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we put anything here? |
||
| }) | ||
| .catch(err => { | ||
| log.error(err); | ||
| }); | ||
| } | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,7 @@ var minimist = require('minimist'); | |
| var s3urls = require('s3urls'); | ||
| var Dyno = require('@mapbox/dyno'); | ||
| var crypto = require('crypto'); | ||
| var AWS = require('aws-sdk'); | ||
| var s3 = new AWS.S3(); | ||
| var { S3Client, GetObjectCommand } = require('@aws-sdk/client-s3'); | ||
| var assert = require('assert'); | ||
|
|
||
| var args = minimist(process.argv.slice(2)); | ||
|
|
@@ -34,6 +33,8 @@ if (!table) { | |
| var region = table.split('/')[0]; | ||
| table = table.split('/')[1]; | ||
|
|
||
| var s3Client = new S3Client({ region: region }); | ||
|
|
||
| var s3url = args._[1]; | ||
|
|
||
| if (!s3url) { | ||
|
|
@@ -86,30 +87,51 @@ dyno.getItem({ Key: key }, function(err, data) { | |
| if (err) throw err; | ||
| var dynamoRecord = data.Item; | ||
|
|
||
| s3.getObject(s3url, function(err, data) { | ||
| if (err && err.statusCode !== 404) throw err; | ||
| var s3data = err ? undefined : Dyno.deserialize(data.Body.toString()); | ||
|
|
||
| console.log('DynamoDB record'); | ||
| console.log('--------------'); | ||
| console.log(dynamoRecord); | ||
| console.log(''); | ||
|
|
||
| console.log('Incremental backup record (%s)', s3url.Key); | ||
| console.log('--------------'); | ||
| console.log(s3data); | ||
| console.log(''); | ||
|
|
||
| try { | ||
| assert.deepEqual(s3data, dynamoRecord); | ||
| console.log('----------------------------'); | ||
| console.log('✔ The records are equivalent'); | ||
| console.log('----------------------------'); | ||
| } | ||
| catch (err) { | ||
| console.log('--------------------------------'); | ||
| console.log('✘ The records are not equivalent'); | ||
| console.log('--------------------------------'); | ||
| } | ||
| }); | ||
| s3Client.send(new GetObjectCommand(s3url)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| .then(data => { | ||
| var s3data = Dyno.deserialize(data.Body.toString()); | ||
|
|
||
| console.log('DynamoDB record'); | ||
| console.log('--------------'); | ||
| console.log(dynamoRecord); | ||
| console.log(''); | ||
|
|
||
| console.log('Incremental backup record (%s)', s3url.Key); | ||
| console.log('--------------'); | ||
| console.log(s3data); | ||
| console.log(''); | ||
|
|
||
| try { | ||
| assert.deepEqual(s3data, dynamoRecord); | ||
| console.log('----------------------------'); | ||
| console.log('✔ The records are equivalent'); | ||
| console.log('----------------------------'); | ||
| } | ||
| catch (err) { | ||
| console.log('--------------------------------'); | ||
| console.log('✘ The records are not equivalent'); | ||
| console.log('--------------------------------'); | ||
| } | ||
| }) | ||
| .catch(err => { | ||
| if (err.$metadata && err.$metadata.httpStatusCode === 404) { | ||
| var s3data = undefined; | ||
|
|
||
| console.log('DynamoDB record'); | ||
| console.log('--------------'); | ||
| console.log(dynamoRecord); | ||
| console.log(''); | ||
|
|
||
| console.log('Incremental backup record (%s)', s3url.Key); | ||
| console.log('--------------'); | ||
| console.log(s3data); | ||
| console.log(''); | ||
|
|
||
| console.log('--------------------------------'); | ||
| console.log('✘ The records are not equivalent'); | ||
| console.log('--------------------------------'); | ||
| } else { | ||
| throw err; | ||
| } | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you didnt introduce this pattern but we should be using
constand notvarfor this