Skip to content
This repository has been archived by the owner on Jun 10, 2022. It is now read-only.

Choose partition by key for several brokers #208

Closed
wants to merge 11 commits into from

Conversation

demyanovs
Copy link

Hi,
I have three brokers and when I, for example, write 5 messages in a cycle, they come in random order.
Before choosing a broker we need to select partition by key, so that the messages were written consistently in correct order.

Copy link
Contributor

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

That's a great idea but we do need to have tests here, can you please add them (at least a functional one)?

} else {
$partId = $value['partId'];
shuffle($partNums);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the shuffle needed only for when you're using $partsNum?

$partId = 0;
if (! isset($value['partId']) || ! isset($topicMeta[$value['partId']])) {
$partId = $partNums[0];
if (isset($value['key']) && trim($value['key'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Extracting this logic to a method and using early returns will make things way easier to understand, could you please do that?

@demyanovs
Copy link
Author

I moved the logic of the choice of partitions to the broker method and add a test.

@lcobucci
Copy link
Contributor

@demyanovs awesome, thanks! Can you please address the issues pointed out by phpstan? I'll take a look at your code when the build is green 👍

@lcobucci
Copy link
Contributor

Also, please use master as destination branch 😄

@demyanovs demyanovs changed the base branch from v0.3.0-dev to master May 24, 2018 15:15
@demyanovs demyanovs changed the base branch from master to v0.3.0-dev May 24, 2018 15:19
@demyanovs
Copy link
Author

@lcobucci Thanks for the comments, I closed this one and created new pull request to master #218

@demyanovs demyanovs closed this May 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants