[Event Hubs] Partition load balancer implementation in Event Processor #4839

ShivangiReja · 2019-08-21T00:42:36Z

Implement partition load balancer that would (almost) evenly distribute ownership of Event Hub partitions among all the active Event Processors registered for a given Event Hub + consumer group combination. Each event processor is responsible for processing events from the partition it owns.

This PR is for load balancing Event Processors as per the algorithm - https://gist.github.com/srnagar/7ef8566cfef0673288275c450dc99590

…to implement_loadbalancer

sdk/eventhub/event-hubs/src/eventProcessor.ts

chradek · 2019-08-26T16:42:46Z

sdk/eventhub/event-hubs/src/eventProcessor.ts

@@ -203,6 +206,26 @@ export class EventProcessor {
    this._partitionManager = partitionManager;
    this._processorOptions = options;
    this._pumpManager = new PumpManager(this._id, options);
+    const inactiveTimeLimitInMS = 60000; // ownership expiration time (1 mintue)


Just curious, is this the same for all SDKs?

All the SDKs are using different time. Java is using 5 minutes, python is using 20 seconds.

Can you start a discussion with the other language SDKs to pin down a value to use for this? As long as it's longer than the loop which runs every 10 seconds I'm not too concerned.

Everyone is using longer time than the loop which runs every 10 seconds. I'll start discussion with other language SDKs to make this consistent across all the languages.

sdk/eventhub/event-hubs/src/eventProcessor.ts

sdk/eventhub/event-hubs/src/partitionLoadBalancer.ts

chradek

One question, did we reach a consensus on whether to include partitions we already own when claiming them? I recall discussions around this to prevent the case where no checkpointing has occurred for a long time due to a lack of events being received.

If so this would require more changes, but I'm also fine with that being in a separate PR since I don't think it would require any major refactoring with what you have here.

chradek · 2019-08-28T19:58:18Z

sdk/eventhub/event-hubs/src/eventProcessor.ts

@@ -203,6 +206,26 @@ export class EventProcessor {
    this._partitionManager = partitionManager;
    this._processorOptions = options;
    this._pumpManager = new PumpManager(this._id, options);
+    const inactiveTimeLimitInMS = 60000; // ownership expiration time (1 mintue)


Can you start a discussion with the other language SDKs to pin down a value to use for this? As long as it's longer than the loop which runs every 10 seconds I'm not too concerned.

sdk/eventhub/event-hubs/src/partitionLoadBalancer.ts

sdk/eventhub/event-hubs/src/eventProcessor.ts

sdk/eventhub/event-hubs/src/partitionLoadBalancer.ts

chradek · 2019-08-28T21:09:28Z

sdk/eventhub/event-hubs/src/partitionLoadBalancer.ts

+    }
+    return (
+      numberOfPartitionsOwned < minPartitionsPerEventProcessor ||
+      numberOfPartitionsOwned === leastPartitionsOwnedByAnyEventProcessor


I'm not sure if this second condition makes sense.

Scenario:
There are 6 partitions total, and 4 EventProcessors.
The expected distribution would be 2 EventProcessors owning 2 partitions, and 2 EventProcessors owning 1 partition.

Let's say our EventProcessor currently owns 1 partition. In this case, numberOfPartitionsOwned === leastPartitionsOwnedByAnyEventProcessor will be true, indicating that the EventProcessor should own more partitions. This would be incorrect though since based on the scenario, we know we've reached an optimal load balance.

I think the EventProcessor should own more partitions if:

numberOfPartitionsOwned is less than the minimum partitions per event processor.

The sum of partitions actively owned by any processor is less than the total number of partitions AND the numberOfPartitionsOwned is less than the minPartitionsPerEventProcessor + 1.

We won't reach this code if the load is already balanced. On line 185, there's a check to see if the load is already balanced. This method gets called only when the load is not balanced.

That's true, but I still think it was right to change the logic since the function should still act as expected in isolation. (If we changed the code such that we didn't check _isLoadBalanced first this method should still return the expected result)

I think what you said is true only if _shouldOwnMorePartitions() was public, but I don't think that if the method is private then it has to work in isolation. If anyone will change the code, they should check the logic before using private methods in a class.

chradek · 2019-08-28T21:12:27Z

sdk/eventhub/event-hubs/src/partitionLoadBalancer.ts

+
+    if (!this._shouldOwnMorePartitions(minPartitionsPerEventProcessor, ownerPartitionMap)) {
+      log.partitionLoadBalancer(
+        `[${this._ownerId}] This event processor owns ${ownerPartitionMap.get(


Do you mean to print the number of partitions owned, or the array? Right now I think it will do the array and not the length.

sdk/eventhub/event-hubs/src/partitionLoadBalancer.ts

ShivangiReja · 2019-08-29T00:26:23Z

One question, did we reach a consensus on whether to include partitions we already own when claiming them? I recall discussions around this to prevent the case where no checkpointing has occurred for a long time due to a lack of events being received.

If so this would require more changes, but I'm also fine with that being in a separate PR since I don't think it would require any major refactoring with what you have here.

I think we didn't reach to any conclusion that day. I'll check with other languages SDKs and create a separate PR for that.

chradek · 2019-08-29T16:25:54Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2019-08-29T16:26:03Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/eventhub/event-hubs/src/eventProcessor.ts

sdk/eventhub/event-hubs/src/partitionLoadBalancer.ts

chradek

Only other comment would be to add tests around load balancing for the cases mentioned in the PR, otherwise LGTM.

ShivangiReja added 11 commits August 12, 2019 16:35

Claim partition ownership in case of single instance

048d998

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…

e895150

…to implement_loadbalancer

Partition load balancer implementation

54ec63f

Update lastModifiedTime when claiming partition ownership

ecaae97

Add JS docs

9826da3

Code cleanup

5bb6f44

Add test

45489b6

Add and update tests

1e5fbe3

Format files

9027930

update ownership expiration time

584b76e

Code cleanup

1300864

ShivangiReja requested review from chradek and ramya-rao-a August 21, 2019 18:31

ShivangiReja added 8 commits August 21, 2019 13:55

Change expiration time

15ceb8b

Uncomment test casese

d1082f2

Format file

a011b88

Update the closeReason, If the partition was stolen

21b674a

Fix test

7374016

Code cleanup

569f2ea

Add logs

40106fe

Improve Event processor doc

1452b76

chradek suggested changes Aug 26, 2019

View reviewed changes

ShivangiReja marked this pull request as ready for review August 27, 2019 18:21

Address comments

419e574

ShivangiReja self-assigned this Aug 28, 2019

ShivangiReja added Client This issue points to a problem in the data-plane of the library. Event Hubs labels Aug 28, 2019

chradek reviewed Aug 28, 2019

View reviewed changes

Address comments

755d27f

chradek reviewed Aug 29, 2019

View reviewed changes

sdk/eventhub/event-hubs/src/eventProcessor.ts Show resolved Hide resolved

sdk/eventhub/event-hubs/src/partitionLoadBalancer.ts Outdated Show resolved Hide resolved

ShivangiReja and others added 3 commits August 29, 2019 10:45

Add the abortSignal.aborted check

3363b5c

Merge remote-tracking branch 'origin/master' into pr/ShivangiReja/4839

7fe962d

Simplify load balancer class

0b16664

chradek approved these changes Aug 29, 2019

View reviewed changes

ShivangiReja merged commit 97b2f75 into Azure:master Aug 29, 2019

ShivangiReja mentioned this pull request Sep 10, 2019

[Event Hubs] Partition load balancer implementation in Event Processor #4742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Event Hubs] Partition load balancer implementation in Event Processor #4839

[Event Hubs] Partition load balancer implementation in Event Processor #4839

ShivangiReja commented Aug 21, 2019

chradek Aug 26, 2019

ShivangiReja Aug 27, 2019

chradek Aug 28, 2019

ShivangiReja Aug 29, 2019

chradek left a comment

chradek Aug 28, 2019

chradek Aug 28, 2019

ShivangiReja Aug 30, 2019

chradek Aug 30, 2019

ShivangiReja Aug 31, 2019

chradek Aug 28, 2019

ShivangiReja commented Aug 29, 2019

chradek commented Aug 29, 2019

azure-pipelines bot commented Aug 29, 2019

chradek left a comment

[Event Hubs] Partition load balancer implementation in Event Processor #4839

[Event Hubs] Partition load balancer implementation in Event Processor #4839

Conversation

ShivangiReja commented Aug 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShivangiReja commented Aug 29, 2019

chradek commented Aug 29, 2019

azure-pipelines bot commented Aug 29, 2019

chradek left a comment

Choose a reason for hiding this comment