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

NIFI-2608 reworked consumer to match API. Added support for 0.10 client in addition. More notes on JIRA #930

Closed
wants to merge 2 commits into from

Conversation

joewitt
Copy link
Contributor

@joewitt joewitt commented Aug 24, 2016

No description provided.

@bbende
Copy link
Contributor

bbende commented Aug 25, 2016

I've been reviewing this... so far I have tested ConsumeKafka and PublishKafka (0.9.0) against an Apache Kafka 0.9.0 broker, with PLAINTEXT and also with SSL, and both appear to be working well. Trying to setup a Kerberos scenario to test SASL.

@bbende
Copy link
Contributor

bbende commented Aug 25, 2016

Quick update... I was finally able to get a Kerberos environment setup and did a successful test with SASL, so that covers the 0.9.0 client against the 0.9.0 broker, with PLAINTEXT, SSL, and SASL.

I'm planning to do the same tests with the 0.10 client and 0.10 broker tomorrow.
So far things are looking good though!

@joewitt
Copy link
Contributor Author

joewitt commented Aug 25, 2016

Update on weird behavior with subclassing. Turns out I can run either 0.9 or 0.10. Whichever runs first wins. There is clearly something going on with classloading. Looking into Kafka source code and what we're doing as well.

@joewitt
Copy link
Contributor Author

joewitt commented Aug 25, 2016

Problem identified. Filed as NIFI-2660

Signed-off-by: joewitt <joewitt@apache.org>
*/
public static NarCloseable withComponentNarLoader(final Class componentClass) {
final ClassLoader current = Thread.currentThread().getContextClassLoader();
Thread.currentThread().setContextClassLoader(NarThreadContextClassLoader.getInstance());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line is superfluous - we are immediately overwriting the context class loader in the next line.

@bbende
Copy link
Contributor

bbende commented Aug 25, 2016

Using the latest updates I was able to verify the 0.10 processors against the 0.10 broker using PLAINTEXT, SSL, and SASL, and also tested the 0.9.0 processors against an 0.10 broker.

I'm a +1 to merge this in.

@markap14
Copy link
Contributor

@joewitt I've not reviewed the entire PR here, but did review the commit for the changes to class loading, and I believe these changes to be correct. I made one in-line comment but other than that I am a +1 to merge to master. Nice catch there. I think it definitely is a cleaner and more correct approach.

…mer API. Made nar classloading more precise to support spawned threads NIFI-2660.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants