Update built-in partitioner options to include murmur2 #396
Conversation
|
Travis failure due to the lingering 0.11.4 version in |
|
There's no need for this anymore since librdkafka's partitioners can be configured directly in librdkafka through string properties, search for "partitioner" here: https://github.com/edenhill/librdkafka/blob/master/CONFIGURATION.md We should rather remove this code from the Python client |
|
Makes sense, hadn't considered the fall through to librdkafka. Update inbound |
edenhill
left a comment
edenhill
left a comment
There was a problem hiding this comment.
I think you can remove all the partitioner functionality, just grep for partitioner and you'll find a horde of unused stuff.
tests/test_Producer.py
Outdated
| p.flush() | ||
|
|
||
|
|
||
| def test_set_partioner_murmur2(): |
tests/test_Producer.py
Outdated
| """ | ||
| with pytest.raises(KafkaException) as e: | ||
| Producer({'partitioner': 'murmur'}) | ||
| assert e == 'unknown builtin partitioner: murmur' in e |
There was a problem hiding this comment.
are you certain this works, referencing the exception from with the block?
I think it needs to go in the outer block
There was a problem hiding this comment.
It would fail otherwise no? I have tested it without checking for the raised exception at all and it raised the appropriate exception
There was a problem hiding this comment.
the assert is never reached since Producer() raises an exception
There was a problem hiding this comment.
yep you are correct.
fixing
|
@edenhill, I think I got it all this go around |
tests/test_Producer.py
Outdated
| p.flush() | ||
|
|
||
|
|
||
| def test_set_partioner_murmur2(): |
|
Dont forget retarget for master before merging |
Custom partitioner was removed a long time ago having predefined set of partitioners from librdkafka (confluentinc#396) but 'known bug' note still exists. That note is misleading regarding the use of the 'partitioner' configuration property
2 participants
Referenced in #231