-
Notifications
You must be signed in to change notification settings - Fork 4
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: queue #430
feat: queue #430
Conversation
handler: QueueHandler; | ||
} | ||
|
||
class Queue extends Construct implements IQueue { |
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 don't see it implementing any of the IQueue methods?
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.
IQueue is just properties (above)?
return call.arguments.length === 3 || call.arguments.length === 4; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
/** | ||
* A heuristic for identifying a {@link CallExpression} that is a call to an `on` handler. |
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.
Curious why you decided to allow multiple consumers?
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.
ohh, this is changed, forgot to change the infer.
@@ -38,6 +38,7 @@ export function createTransactionWorker( | |||
const propertyRetriever = new AllPropertyRetriever({ | |||
BucketPhysicalName: unsupportedPropertyRetriever, | |||
OpenSearchClient: unsupportedPropertyRetriever, | |||
QueuePhysicalName: unsupportedPropertyRetriever, |
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.
Why is this unsupported in this context?
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.
Same reason as bucket physical name, the current support for these are through the client, which we don't currently inject into the transaction worker. Would need to decouple the physical name calculation (and overrides) from the client first.
} | ||
|
||
/** | ||
* TODO: implement message deduplication |
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.
Issue or do? Seems straightforward?
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 guess the hard part is the content based deduplication
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.
The issue is the time window, but may not be hard to do. Save all of the values/hashes with a date, separated by group for fifo. Ignore messages with the same value/hash until the time is up.
Config:
Client-side APIs:
Tasks: