[event-hubs] EventHubsBufferedProducerClient skeleton #17761

chradek · 2021-09-21T17:23:54Z

This is part 1 of #17699.

This PR creates the skeleton of the EventHubsBufferedProducerClient and exposes the new APIs.

Implementation will come in a follow-up PR. The skeleton is split from the implementation to keep changes smaller and easier to read.

richardpark-msft · 2021-09-28T20:18:40Z

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

+   * When `false`, abandon all buffered events and close immediately.
+   * Defaults to `false`.
+   */
+  flush?: boolean;


It seems like the default should be 'true', not 'false' since that's the safest way to operate.

Any reason to prefer abandon?

(and if the default flips maybe you could just change the field to 'abandon', so the default-ish value lines up with what you want)

And just to check, I actually thought 'flush' was the default in .NET but that could have changed (CC: @jsquire)

Correct; the intent, based on board feedback, is to prefer flush and require opt-in for abandonment.

If a similar thing will be added to service-bus, "abandon" wouldn't be a good pick since it is already taken.

Ah, I thought I copied what .NET was doing here for flush. @jsquire, do you still call it flush and default to true if the user doesn't specify it then?

We do. (src)

richardpark-msft · 2021-09-28T20:21:25Z

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

+   * @param eventHubName - The name of the specific Event Hub to connect the client to.
+   * @param credential - An credential object used by the client to get the token to authenticate the connection
+   * with the Azure Event Hubs service.
+   * See &commat;azure/identity for creating credentials that support AAD auth.


Oof, can't use @ directly, huh?

We tried...it didn't render correctly in docs. I haven't checked to see if that's improved since the last time we tried though...

richardpark-msft · 2021-09-28T20:22:35Z

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

+   */
+  async close(options: BufferedCloseOptions = {}): Promise<void> {
+    if (options.flush) {
+      await this.flush(options);


So the idea would be that this.flush() is safe to call multiple times? So if 'close' throws an error I should feel free to call close() again?

Yes; flush should be safe to call multiple times.

richardpark-msft · 2021-09-28T20:22:57Z

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

+   * @returns The total number of events that are currently buffered and waiting to be published, across all partitions.
+   */
+  async enqueueEvents(
+    events: EventData[] | AmqpAnnotatedMessage[],


No single message at a time option?

There should be; we want to encourage the single overload as the majority case for callers. The multiple overload has the possibility for partial success; we want to guide callers to the singular overload for scenarios where tracking the success/failure of each event being enqueued is important to the application.

I'll add the singular enqueueEvent method.

@chradek if other languages support single message option as an overload, is it worth it to support it here as such as well?

enqueueEvents(events: EventData | AmqpAnnotatedMessage, options: EnqueueEventOptions): Promise<number>;

Sure, I'll add that in for the preview. For what it's worth, we debated this when looking at the traditional API send API. We got rid of the single event overload there since it seemed easy enough to just wrap an event in an array, but happy to try it out again now that the use case is changed.

Indeed we did. However, this use case is decidedly different and the single event enqueue is the preferred call pattern for most customer scenarios. 😄

HarshaNalluru · 2021-09-28T21:24:02Z

sdk/eventhub/event-hubs/review/event-hubs.api.md

@@ -82,6 +91,29 @@ export interface EventDataBatch {
    tryAdd(eventData: EventData | AmqpAnnotatedMessage, options?: TryAddOptions): boolean;
 }

+// @public
+export class EventHubBufferedProducerClient {


Would this come to service-bus too?

We haven't talked about doing this for service-bus as well yet. Some of the reasons we're doing this for event hubs (e.g. idempotent producer) don't apply to service bus, but service bus is definitely out of scope 😄

HarshaNalluru · 2021-09-28T21:29:45Z

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

+   * @param events - An array of {@link EventData} or `AmqpAnnotatedMessage`.
+   * @param options - A set of options that can be specified to influence the way in which
+   * events are sent to the associated Event Hub.
+   * - `abortSignal`  : A signal the request to cancel the send operation.


Suggested change

* - `abortSignal` : A signal the request to cancel the send operation.

* - `abortSignal` : A signal to the request to cancel the send operation.

?

jsquire · 2021-09-28T21:26:03Z

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

+   * When `false`, abandon all buffered events and close immediately.
+   * Defaults to `false`.
+   */
+  flush?: boolean;


Correct; the intent, based on board feedback, is to prefer flush and require opt-in for abandonment.

jsquire · 2021-09-28T21:27:58Z

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

+  constructor(
+    fullyQualifiedNamespaceOrConnectionString1: string,
+    eventHubNameOrOptions2: string | EventHubBufferedProducerClientOptions,
+    credentialOrOptions3?:


I am so jealous of union types....

We do get the best stuff in TypeScript :D

jsquire · 2021-09-28T21:28:34Z

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

+   */
+  async close(options: BufferedCloseOptions = {}): Promise<void> {
+    if (options.flush) {
+      await this.flush(options);


Yes; flush should be safe to call multiple times.

jsquire · 2021-09-28T21:29:17Z

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

+   * @returns The total number of events that are currently buffered and waiting to be published, across all partitions.
+   */
+  async enqueueEvents(
+    events: EventData[] | AmqpAnnotatedMessage[],


There should be; we want to encourage the single overload as the majority case for callers. The multiple overload has the possibility for partial success; we want to guide callers to the singular overload for scenarios where tracking the success/failure of each event being enqueued is important to the application.

deyaaeldeen · 2021-10-01T22:17:25Z

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

+ * The `EventHubBufferedProducerClient` does not publish events immediately.
+ * Instead, events are buffered so they can be efficiently batched and published
+ * when the batch is full or the `maxWaitTimeInMs` has elapsed with no new events
+ * enqueued.


NIT: Since this paragraph describes the client purpose, I feel like it should be before the one preceding it.

deyaaeldeen · 2021-10-01T22:18:57Z

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

+   * - `webSocketOptions`: Configures the channelling of the AMQP connection over Web Sockets.
+   * - `userAgent`      : A string to append to the built in user agent string that is passed to the service.
+   */
+  constructor(connectionString: string, options: EventHubBufferedProducerClientOptions); // eslint-disable-line @azure/azure-sdk/ts-naming-options


I am surprised @azure/azure-sdk/ts-naming-options needs to be disabled here. What did it expect instead?

I had copy pasted this from the EventHubProducerClient which had EventHubClientOptions as the options type. I'm able to remove this disable comment now.

deyaaeldeen · 2021-10-01T22:31:33Z

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

+      .then((eventHubProperties) => {
+        return eventHubProperties.partitionIds;
+      });


NIT:

Suggested change

.then((eventHubProperties) => {

return eventHubProperties.partitionIds;

});

.then((eventHubProperties) => eventHubProperties.partitionIds);

This will change in the implementation to just return getPartitionIds on a local EventHubProducerClient, so I'll defer for now.

@chradek I do not mind deferring, it is a small nit but I am curious what did you mean? if the two statements do not behave the same, what is the difference?

… false

…ntHubBufferedProducerClientOptions

* [event-hubs] EventHubsBufferedProducerClient skeleton (#17761) * temp transfer * [event-hubs] initial EventHubBufferedProducerClient skeleton * Change default value of BufferedCloseOptions.flush to true instead of false * Reorder doc comment content for EventHubBufferedProducerClient * remove eslint exception for @azure/azure-sdk/ts-naming-options in EventHubBufferedProducerClientOptions * add enqueueEvent (singular) method * update API view * [WIP][event-hubs] EventHubBufferedProducerClient implementation (#18106) * transfer * [mock-hub] fix issue where calling stop() would not remove existing idle connection timeout watchers * [event-hubs] reduce repetition in hubruntime.spec.ts tests * Add support for flush,eventSuccess and eventError handlers, and backpressure to EventHubBufferedProducerClient * add documentation and update API to align with .NET * [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping (#18331) * [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping * fix the implementation * factor out the hashing logic * remove unused import * the test pass but the implementation needs to be simplified * simplified implementation * address feedback * address feedback * fix format * address feedback Co-authored-by: chradek <51000525+chradek@users.noreply.github.com>

* Removing return response for Pause/Resume/Stop recording * Removing validations * Fixes after merge * Added downloadToFile operation * Added downloadToFile test * Updating API MD file * Fixing format auto * Renaming files * [KeyVault] - Move MHSM to resource group location (#18664) Moving the Managed HSM to the same location as our resource group now that the limits have been expanded. * Add default cloud configuration values to source (#18653) Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> * simplify the commit history so that the patch can apply properly (#18665) Co-authored-by: scbedd <45376673+scbedd@users.noreply.github.com> * Updating name for header * [Event Hubs] Merge feature branch for buffered producer (#18590) * [event-hubs] EventHubsBufferedProducerClient skeleton (#17761) * temp transfer * [event-hubs] initial EventHubBufferedProducerClient skeleton * Change default value of BufferedCloseOptions.flush to true instead of false * Reorder doc comment content for EventHubBufferedProducerClient * remove eslint exception for @azure/azure-sdk/ts-naming-options in EventHubBufferedProducerClientOptions * add enqueueEvent (singular) method * update API view * [WIP][event-hubs] EventHubBufferedProducerClient implementation (#18106) * transfer * [mock-hub] fix issue where calling stop() would not remove existing idle connection timeout watchers * [event-hubs] reduce repetition in hubruntime.spec.ts tests * Add support for flush,eventSuccess and eventError handlers, and backpressure to EventHubBufferedProducerClient * add documentation and update API to align with .NET * [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping (#18331) * [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping * fix the implementation * factor out the hashing logic * remove unused import * the test pass but the implementation needs to be simplified * simplified implementation * address feedback * address feedback * fix format * address feedback Co-authored-by: chradek <51000525+chradek@users.noreply.github.com> * [Event Hubs] Prepare release (#18672) * [Event Hubs] Prepare release * remove empty sections * Node doesn't support some samples for smoke test (#18496) * Node doesn't support some samples for smoke test * [search-documents] reprocessed samples with exp. generator Co-authored-by: Will Temple <will@wtemple.net> * Add rlc quickstart guideline (#18503) * add llc quickstart guideline * update format * add documentation about ci.yml * update to resolve some comments * update to resolve some comments * update to resolve comments * updates term * update format * update format * Running Rush update * Sync eng/common directory with azure-sdk-tools for PR 2265 (#18683) * Improve devops logging for link checker * Update eng/common/scripts/Verify-Links.ps1 Co-authored-by: Wes Haggard <Wes.Haggard@microsoft.com> Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com> * Build and test predefined set of packages only in identity pipelines (#18686) * Build and test predefined set of packages only in identity pipelines * Fixing playback testing * Run rushx format Co-authored-by: Maor Leger <maorleger@users.noreply.github.com> Co-authored-by: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com> Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> Co-authored-by: scbedd <45376673+scbedd@users.noreply.github.com> Co-authored-by: Deyaaeldeen Almahallawi <dealmaha@microsoft.com> Co-authored-by: chradek <51000525+chradek@users.noreply.github.com> Co-authored-by: Sarangan Rajamanickam <sarajama@microsoft.com> Co-authored-by: Will Temple <will@wtemple.net> Co-authored-by: Qiaoqiao Zhang <55688292+qiaozha@users.noreply.github.com> Co-authored-by: Wes Haggard <Wes.Haggard@microsoft.com> Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com> Co-authored-by: praveenkuttappan <55455725+praveenkuttappan@users.noreply.github.com>

* [KeyVault] - Move MHSM to resource group location (#18664) Moving the Managed HSM to the same location as our resource group now that the limits have been expanded. * Add default cloud configuration values to source (#18653) Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> * simplify the commit history so that the patch can apply properly (#18665) Co-authored-by: scbedd <45376673+scbedd@users.noreply.github.com> * [Event Hubs] Merge feature branch for buffered producer (#18590) * [event-hubs] EventHubsBufferedProducerClient skeleton (#17761) * temp transfer * [event-hubs] initial EventHubBufferedProducerClient skeleton * Change default value of BufferedCloseOptions.flush to true instead of false * Reorder doc comment content for EventHubBufferedProducerClient * remove eslint exception for @azure/azure-sdk/ts-naming-options in EventHubBufferedProducerClientOptions * add enqueueEvent (singular) method * update API view * [WIP][event-hubs] EventHubBufferedProducerClient implementation (#18106) * transfer * [mock-hub] fix issue where calling stop() would not remove existing idle connection timeout watchers * [event-hubs] reduce repetition in hubruntime.spec.ts tests * Add support for flush,eventSuccess and eventError handlers, and backpressure to EventHubBufferedProducerClient * add documentation and update API to align with .NET * [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping (#18331) * [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping * fix the implementation * factor out the hashing logic * remove unused import * the test pass but the implementation needs to be simplified * simplified implementation * address feedback * address feedback * fix format * address feedback Co-authored-by: chradek <51000525+chradek@users.noreply.github.com> * [Event Hubs] Prepare release (#18672) * [Event Hubs] Prepare release * remove empty sections * Node doesn't support some samples for smoke test (#18496) * Node doesn't support some samples for smoke test * [search-documents] reprocessed samples with exp. generator Co-authored-by: Will Temple <will@wtemple.net> * Add rlc quickstart guideline (#18503) * add llc quickstart guideline * update format * add documentation about ci.yml * update to resolve some comments * update to resolve some comments * update to resolve comments * updates term * update format * update format * Sync eng/common directory with azure-sdk-tools for PR 2265 (#18683) * Improve devops logging for link checker * Update eng/common/scripts/Verify-Links.ps1 Co-authored-by: Wes Haggard <Wes.Haggard@microsoft.com> Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com> * Build and test predefined set of packages only in identity pipelines (#18686) * Build and test predefined set of packages only in identity pipelines * Removed raw response from ContentDownloadResponse * Changing name to RepeatanleContentDownloadResult * Changed name to ContentDownloadResult * Fixes after merging Co-authored-by: Maor Leger <maorleger@users.noreply.github.com> Co-authored-by: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com> Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> Co-authored-by: scbedd <45376673+scbedd@users.noreply.github.com> Co-authored-by: Deyaaeldeen Almahallawi <dealmaha@microsoft.com> Co-authored-by: chradek <51000525+chradek@users.noreply.github.com> Co-authored-by: Sarangan Rajamanickam <sarajama@microsoft.com> Co-authored-by: Will Temple <will@wtemple.net> Co-authored-by: Qiaoqiao Zhang <55688292+qiaozha@users.noreply.github.com> Co-authored-by: Wes Haggard <Wes.Haggard@microsoft.com> Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com> Co-authored-by: praveenkuttappan <55455725+praveenkuttappan@users.noreply.github.com>

chradek added 2 commits September 20, 2021 07:57

temp transfer

86acf5a

[event-hubs] initial EventHubBufferedProducerClient skeleton

dd65bfc

chradek added Client This issue points to a problem in the data-plane of the library. Event Hubs labels Sep 21, 2021

chradek requested review from bterlson, HarshaNalluru, richardpark-msft and xirzec as code owners September 21, 2021 17:23

richardpark-msft reviewed Sep 28, 2021

View reviewed changes

HarshaNalluru reviewed Sep 28, 2021

View reviewed changes

jsquire reviewed Sep 28, 2021

View reviewed changes

deyaaeldeen reviewed Oct 1, 2021

View reviewed changes

chradek added 5 commits October 1, 2021 15:42

Change default value of BufferedCloseOptions.flush to true instead of…

617a581

… false

Reorder doc comment content for EventHubBufferedProducerClient

a6c3058

remove eslint exception for @azure/azure-sdk/ts-naming-options in Eve…

7b58025

…ntHubBufferedProducerClientOptions

add enqueueEvent (singular) method

7aaa1b4

update API view

f3b0938

deyaaeldeen approved these changes Oct 6, 2021

View reviewed changes

chradek merged commit 14105b2 into Azure:feature/eh-buffered-producer Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[event-hubs] EventHubsBufferedProducerClient skeleton #17761

[event-hubs] EventHubsBufferedProducerClient skeleton #17761

chradek commented Sep 21, 2021

richardpark-msft Sep 28, 2021

richardpark-msft Sep 28, 2021

richardpark-msft Sep 28, 2021

jsquire Sep 28, 2021

HarshaNalluru Sep 28, 2021

chradek Sep 29, 2021

jsquire Sep 29, 2021

richardpark-msft Sep 28, 2021

chradek Sep 29, 2021

richardpark-msft Sep 28, 2021

jsquire Sep 28, 2021

richardpark-msft Sep 28, 2021

jsquire Sep 28, 2021

chradek Sep 29, 2021

deyaaeldeen Oct 1, 2021

chradek Oct 4, 2021

jsquire Oct 4, 2021

HarshaNalluru Sep 28, 2021

chradek Sep 29, 2021

HarshaNalluru Sep 28, 2021

jsquire Sep 28, 2021

jsquire Sep 28, 2021

chradek Sep 29, 2021

jsquire Sep 28, 2021

jsquire Sep 28, 2021

deyaaeldeen Oct 1, 2021

deyaaeldeen Oct 1, 2021

chradek Oct 4, 2021

deyaaeldeen Oct 1, 2021

chradek Oct 5, 2021

deyaaeldeen Oct 5, 2021

	* - `abortSignal` : A signal the request to cancel the send operation.
	* - `abortSignal` : A signal to the request to cancel the send operation.

[event-hubs] EventHubsBufferedProducerClient skeleton #17761

[event-hubs] EventHubsBufferedProducerClient skeleton #17761

Conversation

chradek commented Sep 21, 2021

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment