Skip to content

Comments

[Event Hubs] Update the create and send batch as per feedback#4346

Merged
ShivangiReja merged 11 commits intoAzure:masterfrom
ShivangiReja:CreateBatch_Cleanups
Jul 18, 2019
Merged

[Event Hubs] Update the create and send batch as per feedback#4346
ShivangiReja merged 11 commits intoAzure:masterfrom
ShivangiReja:CreateBatch_Cleanups

Conversation

@ShivangiReja
Copy link
Member

@ShivangiReja ShivangiReja commented Jul 18, 2019

This PR updates the following:

  • Throws error Partition key is not supported when using producers that were created using a partition id when creating batch instead of send()
  • Added variable count and getter for count in EventDataBatch which returns number of events in the batch
  • Change size -> sizeInBytes in EventDataBatch
  • In normal case of send(), if an empty array is passed in, exit without any error.

For more details: #4307, #4302

…ing producers that were created using a partition id` when creating batch instead of send()
@ShivangiReja ShivangiReja self-assigned this Jul 18, 2019
ShivangiReja and others added 5 commits July 18, 2019 10:31
Co-Authored-By: Ramya Rao <ramya.rao.a@outlook.com>
Co-Authored-By: Ramya Rao <ramya.rao.a@outlook.com>
Co-Authored-By: Ramya Rao <ramya.rao.a@outlook.com>
…r right after _throwIfSenderOrConnectionClosed
log.error(`[${this._context.connectionId}] Empty array was passed. No events to send.`);
return;
}
if (eventData instanceof EventDataBatch && !eventData.batchMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have count exposed on the batch, maybe we should check the count instead of !eventData.batchMessage. We can also initialize the eventData.batchMessage to an empty Buffer in the constructor for EventDataBatch and remove the undefined from its type. That way no one ever has to check for !eventData.batchMessage.

Thoguhts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we have count exposed on the batch, maybe we should check the count instead of !eventData.batchMessage.

Agreed!

We can also initialize the eventData.batchMessage to an empty Buffer in the constructor for EventDataBatch and remove the undefined from its type

It will not add any extra value, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

The extra value is that we dont have to check for if (eventData.batchMessage) or use eventData.batchMessage! before using that property

Copy link
Member Author

Choose a reason for hiding this comment

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

If we initialize batchMessage = Buffer.alloc(); in the EventDataBatch constructor, It will create a new buffer object of the specified size(in our case empty Buffer object).

Buffer object will use memory, so my point was why waste memory? If It is not adding any extra value other than this check if (eventData.batchMessage) or eventData.batchMessage!

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

@ramya-rao-a ramya-rao-a changed the title [Event Hubs] Throw this error Partition key is not supported when using producers that were created using a partition id when creating batch instead of send() [Event Hubs] Update the create and send batch as per feedback Jul 18, 2019
@ramya-rao-a
Copy link
Contributor

There are some merge conflicts here, can you take a look?

ShivangiReja and others added 3 commits July 18, 2019 13:26
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.

2 participants