-
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
[EPH] Call methods on PartitionProcessor while processing single partition #4467
[EPH] Call methods on PartitionProcessor while processing single partition #4467
Conversation
* @ignore | ||
* log statements for partitionManager | ||
*/ | ||
export const partitionPump = debugModule("azure:event-hubs:partitionPump"); |
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.
👍
} | ||
|
||
async start(partitionId: string): Promise<void> { | ||
if (this._partitionProcessor.initialize) { |
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.
Here it would be safer to replace this check with:
if (this._partitionProcessor.initialize) { | |
if (typeof this._partitionProcessor.initialize === "function") { |
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.
Added the same check when we create partitionProcessor
.
this._isReceiving = false; | ||
try { | ||
if (this._receiver) { | ||
this._receiver.close(); |
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.
May still need a .catch
on this close since it's an async method. It won't get caught in this try/catch unless you await it.
this._receiver.close(); | ||
} | ||
this._abortController.abort(); | ||
if (this._partitionProcessor.close) { |
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 here with typeof check:
if (this._partitionProcessor.close) { | |
if (typeof this._partitionProcessor.close === "function") { |
}; | ||
const partitionProcessor = this._partitionProcessorFactory( | ||
partitionContext, | ||
new CheckpointManager() |
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.
Shouldnt checkpoint manager take partition context and partition manager in its constructor?
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.
We didn't implemented CheckpointManager
yet. After the implementation I'll update the sample.
PartitionContext | ||
} from "@azure/event-hubs"; | ||
|
||
class EventProcessorHost { |
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.
Since we are doing quite some naming changes, we should be careful of the terms we use in the samples. Here, I would suggest SimplePartitionProcessor
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.
Also, we should add the constructor that stores the partition context and then in processEvents
use the partitionId and consumer group name in the console.log()
This way, the user will know how to get the "partition" related info when they process 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.
Updated!!
…to implement_PartitionPump
This change includes a basic working event processor with no load balancing capabilities. It brings up the event processor instance and starts consuming events from single partition. A separate issue will be created to processing multiple partitions from a single EPH instance.
This PR is responsible for:
Create a class called
PartitionPump
which is responsible forAdded one working sample.