-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Perf Tests] [AMQP] Test naming consistency update #26674
[Perf Tests] [AMQP] Test naming consistency update #26674
Conversation
API change check API changes are not detected in this pull request. |
Class: SendTest | ||
Arguments: | ||
- --event-size 1024 --batch-size 100 --parallel 64 | ||
|
||
- Test: subscribe | ||
- Test: receive-events |
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.
Nit: On second thought, I feel like we should also have the -batch
suffix here for a bit more clarity and consistency, especially since looking at the SB tests, we test both receive in batch/or non-batch form. In Python we have EH perf-tests for receive in batchs/non-batches, as well. Not a blocker, and ultimately doesn't matter too much, but WDYT?
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.
receive-queue-messages-batch
in SB is different from receive-events
here.
// receive-queue-messages-batch in SB
messages = await receiver.receiveMessages(...)
// (= Batch)
// receive-queue-messages in SB
// Alternative name: process-queue-messages
receiver.subscribe(
{
processMessage: async (_message: ServiceBusReceivedMessage) => {
.....
},
processError: async (args: ProcessErrorArgs) => {
....
},
}
);
// receive-events in EH
// Alternative name: process-events-batch
receiver.subscribe(
{
processEvents: async (events: ReceivedEventData[], _context: PartitionContext) => {
// ..... events (= Batch)
},
processError: async (error: Error | MessagingError, _context: PartitionContext) => {
.....
},
}
);
Adding batch to EH streaming might be misleading in the context of EH vs SB.
If you want to put "batch" in the name, I would prefer..
receive-queue-messages-batch
in SBprocess-queue-messages
in SBprocess-events-batch
in EH
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.
That works for me :)
For consistent naming across languages
For consistent naming across languages