Skip to content
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

feat: write to carpark #139

Merged
merged 7 commits into from
Aug 17, 2023
Merged

feat: write to carpark #139

merged 7 commits into from
Aug 17, 2023

Conversation

olizilla
Copy link
Contributor

@olizilla olizilla commented Aug 16, 2023

Write to the existing carpark bucket rather than creating a dedicated one for pickup.

Updater the pin status on successful write to s3 from the pickup worker. Previously we'd listen for object_created events and trigger a lambda to update the pin status, but we're moving to a shared bucket, so handle it in the worker.

Key changes

  • Update sst stack to adopt bucket by name rather than create it's own
  • Delete the pinUpdate queue, as we now update the pin directly from the pickup worker
  • Improve the error handling so we fail pins that are too large rather than retying them... as we are updating the pin status from the worker now this is feasible. Previously we'd only mark a pin as failed using the dead-letter queue handler.
  • Fix pickup tests to verify pin status update in the dynamo table.
  • Pass the abort signal for the car fetch to the s3 upload so we abort the upload correctly. this fixes timeout and error handling issues we were seeing in the tests where the fetch timeout and chunk timeout would trigger the abort signal, but an error was not thrown immediately, which was making tests hang.

Part of #136

License: MIT
Signed-off-by: Oli Evans <oli@protocol.ai>
License: MIT
Signed-off-by: Oli Evans <oli@protocol.ai>
@seed-deploy seed-deploy bot temporarily deployed to pr139 August 16, 2023 12:59 Inactive
@seed-deploy
Copy link

seed-deploy bot commented Aug 16, 2023

View stack outputs

@seed-deploy seed-deploy bot temporarily deployed to pr139 August 17, 2023 10:38 Inactive
@olizilla olizilla requested a review from alanshaw August 17, 2023 10:47
Comment on lines -10 to -14
# Indexer base url
# 0 -> all request to Indexer
# 100 -> All request to Pickup
# values in between the balancer is applied (eg. 15 -> 15% of the request to Pickup, 85% to Indexer)
BALANCER_RATE=10
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed as we no longer split requests between pickup and cluster, and the balancer code has gone.

@@ -50,5 +50,5 @@ export async function checkCar (car) {
}
}))
const carCid = createCarCid(sha256)
return { carCid, carSize, report }
return { carCid: carCid.toString(), carSize, report }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're working with cid strings everywhere else in this code, as we're handed it from and sqs message. We log the carCid to loki via pino, and it was serialising the cid object instead of calling toString on it, so we normalise it here.

Comment on lines +58 to +72
async updatePinStatus ({ cid, status = 'pinned' }) {
const res = await this.dynamo.send(new UpdateCommand({
TableName: this.table,
Key: { cid },
ExpressionAttributeNames: {
'#status': 'status'
},
ExpressionAttributeValues: {
':s': status
},
UpdateExpression: 'set #status = :s',
ReturnValues: 'ALL_NEW'
}))
return res.Attributes
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pickup can now update the pin status once it's done

Comment on lines +175 to +190
const fetchTimer = debounce(() => abort(FETCH_TOO_SLOW), fetchTimeoutMs)
const chunkTimer = debounce(() => abort(CHUNK_TOO_SLOW), fetchChunkTimeoutMs)
const clearTimers = () => {
fetchTimer.clear()
chunkTimer.clear()
}
function abort (reason) {
clearTimers()
if (!abortCtl.signal.aborted) {
abortCtl.abort(reason)
}
}

// start the clock!
fetchTimer()
chunkTimer()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start the timers as soon as fetch is called, not as some point later when the stream composer decides to call the streamWatcher fn.


await s3Upload.done()

export async function verify ({ client, validationBucket, destinationBucket, key, cid }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move the uploading to the upload function and have verify just do verification. simples!

})
signal.addEventListener('abort', () => s3Upload.abort())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

important line! can't pass an abort signal to the s3Uploader oh no. but you can call abort in it so we deal with that here, to ensure the stream is destroyed when the abort signal is fired by our fetch an chunk timeouts.

@@ -78,7 +78,7 @@ export async function verifyMessage ({ msg, cars, t, bucket, s3 }) {
try {
const message = msg.body
const index = Number(message.requestid)
if (cars[index].expectedResult === 'success') {
if (cars[index].expectedResult === 'pinned') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

align test expectations with database status values

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

const updatePinQueue = new Queue(stack, 'UpdatePinQueue', {
consumer: {
function: {
handler: 'basic/update-pin.sqsEventHandler',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update-pin.js can be removed?

pickup/lib/s3.js Outdated Show resolved Hide resolved
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
@seed-deploy seed-deploy bot temporarily deployed to pr139 August 17, 2023 12:57 Inactive
License: MIT
Signed-off-by: Oli Evans <oli@protocol.ai>
@seed-deploy seed-deploy bot temporarily deployed to pr139 August 17, 2023 13:03 Inactive
@olizilla olizilla merged commit 8a53bce into main Aug 17, 2023
5 checks passed
@olizilla olizilla deleted the carpark branch August 17, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants