Skip to content

KAFKA-6775: Fix the issue of without init super class's #4859

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

Merged
merged 4 commits into from
Apr 18, 2018

Conversation

jiminhsieh
Copy link
Contributor

Some anonymous classes of AbstractProcessor didn't initialize their superclass. This will not set up ProcessorContext context at AbstractProcessor.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I just double checked the code, and it seems that AbstractProcessor is use on other places, too. Did you double check those, if super.init(context) is called there?

It might also make sense to make AbstractProcessor#context protected?

@@ -609,6 +609,7 @@ private KafkaStreams createKafkaStreams(String topic, final CountDownLatch latch

@Override
public void init(ProcessorContext context) {
super.init(context);
Copy link
Member

Choose a reason for hiding this comment

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

might be better to remove the overwrite method completely?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, if we don't care about the context, we could just use Processor directly instead of AbstractProcessor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions! After saw this snippet:

final AbstractProcessor<String, Long> processor = new AbstractProcessor<String, Long>() {
@Override
public void process(final String key, final Long value) {
context().forward(key + value, key.length() + value);
}
};

I wonder how about only keeping process this overwrite method and removing the other overwrite methods which include init, punctuate, and close? I think it is much clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

@@ -648,6 +649,7 @@ private KafkaStreams createKafkaStreamsWithSink(String topic, final CountDownLat
return new AbstractProcessor<Integer, byte[]>() {
@Override
public void init(ProcessorContext context) {
super.init(context);
Copy link
Member

Choose a reason for hiding this comment

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

as above

@@ -48,6 +48,7 @@
@Override
public void init(final ProcessorContext context) {
System.out.println("initializing processor: topic=" + topic + " taskId=" + context.taskId());
super.init(context);
Copy link
Member

Choose a reason for hiding this comment

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

nit: better to do this as first instruction?

@mjsax mjsax requested review from dguy and mjsax April 12, 2018 22:46
@mjsax mjsax added the streams label Apr 12, 2018
@mjsax
Copy link
Member

mjsax commented Apr 12, 2018

\cc @bbejeck @vvcephei

@jiminhsieh
Copy link
Contributor Author

Thanks for the reviewing and suggestion!

I just double checked the code, and it seems that AbstractProcessor is use on other places, too. Did you double check those, if super.init(context) is called there?

If this meant other anonymous classes of AbstractProcessor like some of the usages at MockProcessorContextTest, those are simply didn't overwrite.

it might also make sense to make AbstractProcessor#context protected?

I am not sure about this since there is AbstractProcessor#context() which returns AbstractProcessor#context. It does the same job as making AbstractProcessor#context protected. It means we have two ways to access AbstractProcessor#context.

@guozhangwang
Copy link
Contributor

LGTM!

@guozhangwang guozhangwang merged commit 6b2be26 into apache:trunk Apr 18, 2018
@jiminhsieh jiminhsieh deleted the ticket/KAFKA-6775 branch April 24, 2018 13:57
jeqo pushed a commit to jeqo/kafka that referenced this pull request May 2, 2018
Some anonymous classes of AbstractProcessor didn't initialize their superclass. This will not set up ProcessorContext context at AbstractProcessor.

Reviewers: Matthias J. Sax <[email protected]>, Guozhang Wang <[email protected]>
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
Some anonymous classes of AbstractProcessor didn't initialize their superclass. This will not set up ProcessorContext context at AbstractProcessor.

Reviewers: Matthias J. Sax <[email protected]>, Guozhang Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants