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

Refactor: partition_assignment_strategy to accept enumerated values #25

Merged
merged 11 commits into from
Mar 26, 2020

Conversation

kares
Copy link
Contributor

@kares kares commented Mar 23, 2020

this is follow-up from: #16 where a partitioner option got introduced which enumerates a list of supported values and maps them to Java class names.

a few Kafka customization options accept class names instead of enumerated values.
IMO this makes the configuration a bit ugly and closely tied to Kafka's client API details.
(e.g. value_serializer => 'org.apache.kafka.common.serialization.StringSerializer')

so here's a review of those, turns out that only one setting partition_assignment_strategy supports multiple values wout a default so it has been refactored to accept an enumerated list of values, which are documented, while also allowing for class-names due compatibility..

options such as key_serializer, value_serializer have a default or do not have much variability, so I've left those as they are for now.

also some docs rendering fixes incorporated (for pending 10.1.0 release)

@elasticsearch-bot elasticsearch-bot self-assigned this Mar 23, 2020
@andsel andsel self-requested a review March 23, 2020 14:19
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

I've put some little notes, but generally LGTM
Do you think that a unit test on method partition_assignment_strategy_class could help to avoid future regressions?

docs/input-kafka.asciidoc Show resolved Hide resolved
lib/logstash/inputs/kafka.rb Outdated Show resolved Hide resolved
@kares
Copy link
Contributor Author

kares commented Mar 23, 2020

Thanks Anread, will look into addressing your concerns.

Do you think that a unit test on method partition_assignment_strategy_class could help to avoid future regressions?

I believe the functional (integration) spec we have is sufficient, since Kafka will try to instantiate the class set from configuration. Of course could the spec could be improved to test specifics of each strategy but a unit spec is a bit cumbersome since we could only assert on class names.

spec/integration/inputs/kafka_spec.rb Outdated Show resolved Hide resolved
Comment on lines 379 to 381
`round_robin`, `sticky` and `cooperative_sticky`. These map to Kafka's corresponding
https://kafka.apache.org/24/javadoc/org/apache/kafka/clients/consumer/ConsumerPartitionAssignor.html[`ConsumerPartitionAssignor`]
implementations.
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to usability for the usual cases.

Are there cases in the wild where users provide their own ConsumerPartitionAssignor implementations? Do we need to document that the fully-qualified path to a class may still be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there cases in the wild where users provide their own ConsumerPartitionAssignor implementations?

there probably aren't since we do not allow loading custom .jar files.
in theory, there might be if users add stuff to class-path themselves.

Do we need to document that the fully-qualified path to a class may still be used?

this also came up during #16 as a partitioner => ... option was added with enumerated values (also accepting class-names). we have the custom class name documented but I think we should remove that part:

The configuration also allows setting a partitioner class name
e.g. org.apache.kafka.clients.producer.UniformStickyPartitioner.

so just like here I am for NOT documenting the possibility to set a class-name (unless we're requested to support loading custom classes).

CHANGELOG.md Outdated Show resolved Hide resolved
@kares kares merged commit 1193fe4 into logstash-plugins:master Mar 26, 2020
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.

4 participants