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

Replace "0" with random Partition key (#7178) #14406

Merged
merged 4 commits into from
Aug 26, 2020
Merged

Conversation

soo-toance
Copy link
Contributor

@soo-toance soo-toance commented Aug 20, 2020

before

var ehClient = EventHubClient.CreateFromConnectionString(connectionString);
var partitionSender = ehClient.CreatePartitionSender("0");

after:

var partitions = await this.GetPartitionsAsync(ehClient);
var partitionId = partitions[new Random().Next(partitions.Length)];
var partitionSender = ehClient.CreatePartitionSender(partitionId);

@ghost ghost added the Event Hubs label Aug 20, 2020
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@soo-toance soo-toance marked this pull request as ready for review August 23, 2020 11:29
@jsquire jsquire self-assigned this Aug 23, 2020
Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

Hi @soo-toance. Thank you very much for your contribution. I left one minor suggestion, but the changes look good to me, overall. If you don't mind addressing the one open comment, I'd be happy to merge this in.

try
{
var partitions = await GetPartitionsAsync(ehClient);
var partitionId = partitions[new Random().Next(partitions.Length)];
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend hoisting the Random instance here to a class-level member. While this is the only place that it is used at the moment, an ephemeral Random could be easy to overlook and isn't typically a good pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hello : ) thanks for the suggestion! i'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hello @jsquire i fix it below. (e612a8b) Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @soo-toance! The change looks good to me, I'll go ahead and merge things in. Thank you, again, for your contribution and helping to improve the Azure SDKs .

@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. Community Contribution Community members are working on the issue Event Hubs labels Aug 23, 2020
@jsquire
Copy link
Member

jsquire commented Aug 23, 2020

Fixes #7178

@jsquire jsquire merged commit a665460 into Azure:master Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Community Contribution Community members are working on the issue Event Hubs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants