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

Remove unused skip_double_compressed_messages #1677

Merged
merged 1 commit into from
Jan 13, 2019

Conversation

jeffwidman
Copy link
Contributor

@jeffwidman jeffwidman commented Dec 13, 2018

This skip_double_compressed_messages flag was added in #755 in
order to fix #718.

However, grep'ing through the code, it looks like it this is no longer
used anywhere and doesn't do anything.

So removing it.


This change is Reviewable

@jeffwidman
Copy link
Contributor Author

jeffwidman commented Dec 13, 2018

Note: This is a backwards-incompatible change.

If someone creates a consumer with this kwarg after we remove it, then KafkaConsumer will raise an exception:

# Only check for extra config keys in top-level class
extra_configs = set(configs).difference(self.DEFAULT_CONFIG)
if extra_configs:
raise KafkaConfigurationError("Unrecognized configs: %s" % (extra_configs,))

The simplest solution is just wait to merge this until the 2.0 release.

@jeffwidman jeffwidman added this to the 2.0 milestone Dec 13, 2018
This `skip_double_compressed_messages` flag was added in #755 in
order to fix #718.

However, grep'ing through the code, it looks like it this is no longer
used anywhere and doesn't do anything.

So removing it.
@jeffwidman jeffwidman force-pushed the remove-unused-skip_double_compressed_messages branch from 169535a to b25f4cc Compare December 13, 2018 23:32
@dpkp
Copy link
Owner

dpkp commented Jan 13, 2019

I think it is fine to land now. If you want to leave in a default value (undocumented / unused) to avoid the configuration error, that's reasonable. The supporting code for this config was removed in #1252 / commit fbea5f04bccd28f3aa15a1711548b131504591ac, so the config is and has been a no-op for a while.

@jeffwidman
Copy link
Contributor Author

Sounds good, I'd rather land it now.

@jeffwidman jeffwidman merged commit e54f5a3 into master Jan 13, 2019
@jeffwidman jeffwidman deleted the remove-unused-skip_double_compressed_messages branch January 13, 2019 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants