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 old SimpleClient / SimpleProducer / SimpleConsumer #1196

Merged
merged 1 commit into from
Oct 11, 2019

Conversation

jeffwidman
Copy link
Collaborator

@jeffwidman jeffwidman commented Aug 30, 2017

A bunch of old code cleanup as part of bumping to version 2.

Notably this will drop our test runs from ~6-7 minutes to ~2-3 minutes per run... really convenient.

Fix #1193


This change is Reviewable

@jeffwidman
Copy link
Collaborator Author

jeffwidman commented Oct 22, 2018

I spent a while updating this last night, there are a few tests of the new KafkaConsumer / KafkaProducer that depend on fixtures using the old SimpleClient that still need updating, once that's complete this will be close to done.

It is refreshing poking through the codebase on this branch as it's immediately obvious where things are w/o the confusion of wondering if I'm looking at code used by the old clients or the new clients.

@jeffwidman jeffwidman changed the title WIP: Remove old SimpleClient / SimpleProducer / SimpleConsumer Remove old SimpleClient / SimpleProducer / SimpleConsumer Oct 22, 2018
@jeffwidman jeffwidman force-pushed the 1193-remove-old-simple-clients branch from 9bbf96a to 73b2491 Compare October 22, 2018 21:41
@dpkp
Copy link
Owner

dpkp commented Oct 23, 2018

This is definitely a big one. Re sequencing, I would love to get a final 1.4.X patch release out with any remaining bugfixes for stability. Then we can release this as either 1.5.0 or 2.0.

@jeffwidman
Copy link
Collaborator Author

IMHO this should be a 2.0 release. Massive breakage everywhere. It would pair well with the new Admin client for the 2.0 release.

And 👍 on a bugfix release for the 1.4.X series... I am actually just about to start profiling some code that hit a massive performance regression after switching from SimpleProducer to KafkaProducer, so may have one more more items to land in that 1.4.x series... I think it'd be fine to sneak in the initial admin client as well into this 1.4.x release but the big emphasis of it would be the 2.0 release.

@jeffwidman jeffwidman force-pushed the 1193-remove-old-simple-clients branch 5 times, most recently from f2e21ee to f9e36d0 Compare October 27, 2018 08:22
@jeffwidman jeffwidman force-pushed the 1193-remove-old-simple-clients branch 5 times, most recently from bb6049e to a79775c Compare October 29, 2018 06:11
@jeffwidman jeffwidman force-pushed the 1193-remove-old-simple-clients branch 2 times, most recently from 29b2b08 to 9756d51 Compare August 22, 2019 01:50
@jeffwidman
Copy link
Collaborator Author

jeffwidman commented Aug 22, 2019

I think tests should now fully pass for this, assuming Travis passes for #1886.

There is one commit of TODO's highlighting possible additional code deletions... anything there should probably be included in this PR as well...

Also, still need to update the docs to clearly mention the deprecation/deletion of the old clients.

@jeffwidman jeffwidman force-pushed the 1193-remove-old-simple-clients branch 7 times, most recently from cdc7b15 to c1b4786 Compare August 23, 2019 04:30
@jeffwidman
Copy link
Collaborator Author

Tests are green.

@jeffwidman
Copy link
Collaborator Author

Just rebased to pickup the new updates from the 1.4.7 release.

Hoping we can get this merged in the next few weeks so that it doesn't keep hanging out there needing to be rebased... and so we don't have folks accidentally creating unittest-based tests.

@jeffwidman
Copy link
Collaborator Author

The tests are now failing because more unittest-style tests crept in here: #1833

So those need to be migrated for this to work. Perhaps @ulrikjohansson can handle it when he fixes up his tests.

@jeffwidman jeffwidman force-pushed the 1193-remove-old-simple-clients branch from 14a02a2 to da97f64 Compare October 7, 2019 18:19
@jeffwidman
Copy link
Collaborator Author

Many thanks to @ulrikjohansson for migrating the KafkaAdminClient tests to pytest.

I just rebased this, hopefully Travis works fine.

In the 2.0 release, we're removing:
 * `SimpleClient`
 * `SimpleConsumer`
 * `SimpleProducer`
 * Old partitioners used by `SimpleProducer`; these are superceded by
 the `DefaultPartitioner`

These have been deprecated for several years in favor of `KafkaClient`
/ `KafkaConsumer` / `KafkaProducer`.

Since 2.0 allows breaking changes, we are removing the deprecated
classes.

Additionally, since the only usage of `unittest` was in tests for these
old Simple* clients, this also drops `unittest` from the library. All
tests now run under `pytest`.
@jeffwidman
Copy link
Collaborator Author

It bothers me that we've had two folks nicely submit PR's and then I had to ask them to update tests to drop the Unittest support so that they didn't block this... so I'm planning to merge this.

I think at this point we should do a 2.x version, as it's a big change... I realize we don't religiously follow Semvar, but I don't think 1.5 is a big enough bump.

I filed #1926 to track the documentation of the change for old users.

@mmenbawy
Copy link
Contributor

mmenbawy commented Jul 12, 2020

@jeffwidman @dpkp
Why were the following 3 files removed by this PR?

  • kafka/partitioner/base.py
  • kafka/partitioner/hashed.py
  • kafka/partitioner/roundrobin.py

@ekeric13
Copy link

@jeffwidman @dpkp yeah i am trying to use a round robin partitioner and it is gone now? how come it was removed? was it buggy?

@wbarnha wbarnha mentioned this pull request Aug 8, 2023
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.

Remove old Simple clients
5 participants