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

[client] add properties to producer for cpp & python client #2420

Merged
merged 2 commits into from
Aug 22, 2018

Conversation

sijie
Copy link
Member

@sijie sijie commented Aug 21, 2018

Motivation

This is a caught-up change to enable properties for producer as java clients.

Changes

Enable properties on producer for both cpp & python client

Results

Properties are added as metadata for CommandProducer. However there is no way
to verify the producer properties. so I didn't add any specific tests, just
adding properties for both cpp and python clients in the tests, that should
excerise the corresponding code path.

 ### Motivation

This is a caught-up change to enable properties for producer as java clients.

 ### Changes

Enable properties on producer for both cpp & python client

 ### Results

Properties are added as metadata for CommandProducer. However there is no way
to verify the producer properties. so I didn't add any specific tests, just
adding properties for both cpp and python clients in the tests, that should
excerise the corresponding code path.
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

LGTM, just minor comment on pydoc for new argument

@@ -321,7 +321,8 @@ def create_producer(self, topic,
batching_max_messages=1000,
batching_max_allowed_size_in_bytes=128*1024,
batching_max_publish_delay_ms=10,
message_routing_mode=PartitionsRoutingMode.RoundRobinDistribution
message_routing_mode=PartitionsRoutingMode.RoundRobinDistribution,
properties=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add properties to the pydoc string below

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah will add

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@sijie
Copy link
Member Author

sijie commented Aug 22, 2018

@merlimat I addressed your comment. can you review it again?

@merlimat merlimat merged commit 834104b into apache:master Aug 22, 2018
sijie added a commit that referenced this pull request Aug 27, 2018
* [client] add properties to producer for cpp & python client

 ### Motivation

This is a caught-up change to enable properties for producer as java clients.

 ### Changes

Enable properties on producer for both cpp & python client

 ### Results

Properties are added as metadata for CommandProducer. However there is no way
to verify the producer properties. so I didn't add any specific tests, just
adding properties for both cpp and python clients in the tests, that should
excerise the corresponding code path.

* Add `properties` to pydoc
@sijie
Copy link
Member Author

sijie commented Aug 27, 2018

cherry-pick as d7ec1c5 in branch-2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants