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

kafka-clients 3.0.0 Compatibility #239

Merged
merged 3 commits into from
Sep 13, 2021
Merged

Conversation

garyrussell
Copy link
Contributor

The upcoming 3.0.0 release adds a new method to Consumer and removes
a deprecated one from ConsumerRecord.

Use reflection for these two methods to enable reactor-kafka to be used
with the older clients as well as 3.0.0 when used in other projects, such
as Spring for Apache Kafka.

The upcoming 3.0.0 release adds a new method to `Consumer` and removes
a deprecated one from `ConsumerRecord`.

Use reflection for these two methods to enable reactor-kafka to be used
with the older clients as well as 3.0.0 when used in other projects, such
as Spring for Apache Kafka.
Copy link
Contributor

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Some minor nit-picks, otherwise LGTM

import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.locks.ReentrantLock;
import java.util.regex.Pattern;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this Project Reactor code style to have Java imports in the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; this class was not consistent.

// Consumer delegate = mock(Consumer.class);
// ConsumerDelegate consumer = new ConsumerDelegate(delegate);
// given(delegate.currentLag(any())).willReturn(OptionalLong.of(42L));
// assertEquals(42L, consumer.currentLag(new TopicPartition("foo", 0)).getAsLong());
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you going to finish this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It requires the 3.0.0 client on the classpath - I used it to test with that client - commented out for older clients.

Copy link
Contributor

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

LGTM, but:

Only those with write access to this repository can merge pull requests.

So, sorry, I can't merge this for you at the moment.

Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

sorry this one fell through the cracks. lgtm, and I'll also defer to @artembilan review 👍

@simonbasle simonbasle added this to the 1.3.6 milestone Sep 13, 2021
@simonbasle simonbasle merged commit 2007b85 into reactor:main Sep 13, 2021
@garyrussell garyrussell deleted the 300WIP branch September 28, 2021 20:26
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.

3 participants