Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added RackAffinityGroupBalancer strategy #361
Added RackAffinityGroupBalancer strategy #361
Changes from 1 commit
9052d9c
60bf86a
adf0e49
40845b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making
findRack
an exported function such asRackResolverAWS
so that client code can explicitly call it from their own resolver.It's also a good candidate for living in its own package, alongside other common cloud providers (just look at how many imports you dragged in with this one PR! Even though they're all stdlib.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! On further reflection, I'm actually planning to pull out the AWS-specific code. If I left it in, then I think the suggestion to export
RackResolverAWS
is a good one. However, that got me to thinking that, from the perspective of exporting a clean and focused public API, AWS code has no place in kafka-go. That got me thinking even more, and I think that it's weird for the code to use the AWS logic quietly and by default.That said, I can provide this code as an example and make it available to the community that way. Maybe someday someone will create an open source, multi-cloud library that can be plugged in as a rack resolver. Until then, I think it's better to keep the cloud provider code out of kafka-go. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW : While on aws this may not work for teams running on ecs and blocking the ec2 metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I also found that there's a better way to do this on ECS, so I pushed up some new code. Now it will try to use the local ECS metadata and will fail back to trying to EC2 if that doesn't work out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !