-
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] [Service Bus] [Event Hubs] Event Based SDKs Perf Framework - part 2 #18859
[Perf] [Service Bus] [Event Hubs] Event Based SDKs Perf Framework - part 2 #18859
Conversation
… harshan/issue/18471-event-perf-framework-part-2
… harshan/issue/18471-event-perf-framework-part-2
… harshan/issue/18471-event-perf-framework-part-2
API changes have been detected in |
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.
Overall looks good to me. I have some comments (mostly stylish)
export interface SubscribeOptions { | ||
/** | ||
* The maximum number of partitions to receive from. | ||
* - **Default**: `12`. |
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 how the default is determined?
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.
it was just a random number, do you have any preference/suggestion?
/** | ||
* Raises the error after the specified time. | ||
*/ | ||
raiseErrorAfterInSeconds: number; |
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: does raiseErrorDelayInSeconds
read better?
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.
delayToRaiseErrorInSeconds
probably too long...
- Methods do not perform any async work, and sync methods add less overhead to perf framework
- Now matches NoOpTest * AverageBatchSize - Also update options to match .NET
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.
Please fix raiseErrorAfterInSeconds
and I can continue reviewing.
}, | ||
maxEventsPerSecond: { | ||
description: "Max rate to receive in events/sec", | ||
defaultValue: 1000, |
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.
maxEventsPerSecond
should default to unspecified (unthrottled)
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.
}, | ||
raiseErrorAfterInSeconds: { | ||
description: "Raises error after the given duration - in seconds", | ||
defaultValue: 15, |
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.
raiseErrorAfterInSeconds
should default to unspecified (error never thrown).
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.
Updated
this.handlePartition(handlers.processEvents, i, maxEventsPerSecondPerPartition) | ||
); | ||
} | ||
if (options?.raiseErrorAfterInSeconds) { |
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.
raiseErrorAfterInSeconds
does not appear to be working for me. If I set this to say 3
, the test keeps running for the normal duration and no error is raised.
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.
If I set this to say 3, the test keeps running for the normal duration and no error is raised.
Error should have been thrown(or logged at 3 seconds), but the test is active since the duration timeout wasn't cleared.
Updated the "program" to clear the timeout once the runAll calls were done.
… harshan/issue/18471-event-perf-framework-part-2
…ps://github.com/HarshaNalluru/azure-sdk-for-js into harshan/issue/18471-event-perf-framework-part-2
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.
Approved pending remaining feedback
{ | ||
processEvents: async (events: ReceivedEventData[], _context: PartitionContext) => { | ||
for (const _event of events) { | ||
console.log(_event.sequenceNumber, _context.partitionId); |
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.
Remove debugging console.log
); | ||
await producer.sendBatch(batch); | ||
numberOfEventsSent = numberOfEventsSent + batch.count; | ||
console.log(numberOfEventsSent); |
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.
Remove debugging console.log
const batch = await producer.createBatch({ partitionId }); | ||
let numberOfEventsSent = 0; | ||
// add events to our batch | ||
while (numberOfEventsSent <= numberOfEventsPerPartition) { |
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.
while (numberOfEventsSent <= numberOfEventsPerPartition) { | |
while (numberOfEventsSent < numberOfEventsPerPartition) { |
Off-by-one bug
What's in the PR?
Fixes #18471
Fixes #17952