Skip to content
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

Exposing EventHubClient to support sending Partition Key. Fixes #1643. #2225

Merged
merged 1 commit into from
Jun 17, 2019

Conversation

alrod
Copy link
Member

@alrod alrod commented Jun 11, 2019

No description provided.

@@ -72,6 +73,9 @@ public void Initialize(ExtensionConfigContext context)
context.AddBindingRule<EventHubAttribute>()
.BindToCollector(BuildFromAttribute);

// register EventClient output binding
Copy link
Member

@mathewc mathewc Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd just remove this comment. It's not technically an output binding, it's a client/service input binding

private readonly object _messageSender;
private readonly Type _type;

public EventHubClientBinder(object messageSender, Type type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering why this is called "messageSender" when it is an EventClient? Why is the type you use object, and not EventClient?

@@ -72,6 +73,9 @@ public void Initialize(ExtensionConfigContext context)
context.AddBindingRule<EventHubAttribute>()
.BindToCollector(BuildFromAttribute);

// register EventClient output binding
context.AddBindingRule<EventHubAttribute>().BindToValueProvider(GetEventClient);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be using BindToInput instead. See other examples in other extensions, which use this for returning "clients". Here's an example: https://github.com/Azure/azure-webjobs-sdk-extensions/blob/b7aa8f84c2b3c9065861fe955ce8180b9d19f939/src/WebJobs.Extensions.CosmosDB/Config/CosmosDBExtensionConfigProvider.cs#L58

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you'd have something like:

context.AddBindingRule<EventHubAttribute>()
    .BindToInput<EventHubClient>(a =>
    {
        return _options.Value.GetEventHubClient(a.EventHubName, a.Connection);
    });

@@ -127,41 +129,107 @@ public static void SendEvents_TestHub2(int numEvents, string input, [EventHub(Te
}
}
}
}

public class EventHubPartitionKeyEndToEndTests : IClassFixture<TestFixture<EventHubPartitionKeyEndToEndTests.EventHubTestJobs>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you creating a separate end to end test suite for this partition test? Did you run into problems? We shouldn't need a separate full test suite for each new test scenario we want to add.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I want to reuse a eventhub for the test and I do not want create a new eventhub. If I add everything in one class another function can be triggered on new event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if you consume all the events after each test that would solve the issue, right? Ensuring the checkpoint is moved to the "end". Even with your new test suite, it seems to me that you can get leakage across suites. E.g. if one suite wrote a bunch of events that didn't get fully processed (e.g. due to failures or something, or because the tests passed but didn't consume all events), the other suite will see them in the hub when it runs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the other suite will see unprocessed messages. So you propose just to create the other eventhub for the test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don’t want a new event hub per test, nor do we want a new test suite per test :) We need to find a way to clear/consume the messages before each test so each test starts clean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the tests:

  1. Use one test suite.
  2. Use one eventhub.
  3. Filter messages for the current test is using.

@alrod alrod requested a review from mathewc June 17, 2019 17:56
@alrod alrod merged commit c17b391 into Azure:dev Jun 17, 2019
@mvsmal
Copy link

mvsmal commented Jul 18, 2019

Hello @alrod. This change is mentioned in release 3.0.10, however that was a release of the Microsoft.Azure.WebJobs package and this change needs to be released in the Microsoft.Azure.WebJobs.Extensions.EventHubs package. And the latter still has version 3.0.5, which doesn't contain this fix. When do you plan to release it? Or am I missing something? Thanks.

@alrod
Copy link
Member Author

alrod commented Jul 23, 2019

Hi @mvsmal,
Yes, I just published:
https://www.nuget.org/packages/Microsoft.Azure.WebJobs.Extensions.EventHubs/3.0.6

The package should have "EventHubClient" change.
Thanks for heads up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants