Remove SNS #68
Merged
Remove SNS #68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zekehuntergreen
approved these changes
Mar 19, 2024
| @@ -28,7 +28,6 @@ export const getParameters = async ( | |||
| } while (nextToken); | |||
|
|
|||
| if (parameters) { | |||
| logger.info('Fetched parameters from Parameter Store'); | |||
| publishTranscriptionOutputFailure( | ||
| snsClient, | ||
| config.app.destinationTopicArns.transcriptionService, | ||
| await publishTranscriptionOutputFailure( |
There was a problem hiding this comment.
good call - it's probably important to await the result of this before updating scale in protection
| @@ -79,7 +79,7 @@ export const getConfig = async (): Promise<TranscriptionConfig> => { | |||
| const destinationTopic = findParameter( | |||
There was a problem hiding this comment.
destinationQueueUrls ?
Comment on lines
+6
to
+7
| messageId: z.string(), | ||
| body: stringToJSONSchema.pipe(TranscriptionOutput), |
There was a problem hiding this comment.
non-blocking: I'm not sure I understand these changes. output-handler is still reading off of the same sqs queue, right? Why has the shape of the message changed?
There was a problem hiding this comment.
This was unexpected for me as well - I had to work out the structure of the new message by printing it out but I think think previously you'd have an sqs message wrapped around an sns notification, whereas now you've got an sqs message wrapped around the message itself.
marjisound
force-pushed
the
pm-remove-sns
branch
from
32bc404
to
0b85e34
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
Initially when we started this project we had an idea to have the worker instances post to a single SNS topic which then feeds into multiple sqs topics for different services (giant, composer etc). Later, we decided that we didn't want to mix up output destined for giant with output for the transcription-service-output-handler - we shouldn't share data with services that aren't supposed to be consuming it,
Having SNS provided some benefit of easy email notifications when a worker had finished a transcription, though again for privacy/spam reasons we've switched this off #65
However, when trying to integrate the transcription service with giant I found that using SNS added some complexity:
So, given that we're not getting much benefit out of SNS, this PR removes it from the transcription service
How to test
I've tested this on CODE and verified that transcription still works