KAFKA-4568: Simplify test code for multiple SASL mechanisms#2373
KAFKA-4568: Simplify test code for multiple SASL mechanisms#2373rajinisivaram wants to merge 2 commits intoapache:trunkfrom
Conversation
|
@ijuma Can you take a look please? Thank you. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
+1 |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
Thanks for the PR, looks good overall, left a few minor comments.
There was a problem hiding this comment.
Does it ever make sense to pass serverSaslMechanism and dynamicJaasConfig? If not, maybe we should have a clientSaslProperties method.
There was a problem hiding this comment.
Not at the moment, but we may want to support the property in future. But it does make sense to use different methods for server and client properties. Have made that change.
There was a problem hiding this comment.
Thanks, it looks much cleaner to me after your change.
There was a problem hiding this comment.
Nit: Some is a little better than Option in the two cases above. The Option factory method is typically used when null should be converted to None.
There was a problem hiding this comment.
Introducing this makes the setJaasConfiguration method cleaner, but it has two disadvantages:
- Each var can be mutated from a subclass even though it seems like we never want to do that. We can fix that by making the
varprivate and exposing it via adef. - Each var could be accessed before it's initialised and it may not be obvious. We can improve that by making them
nullto start with so that if someone accesses them before they're initialised, they'll get a NPE. We could do even better by adding some code to the accessor, but it doesn't seem worth it.
There was a problem hiding this comment.
@ijuma Thank you for the review. Don't think we need an accessor since subclasses don't use them, so I have made the vars private. I have done 2).
There was a problem hiding this comment.
That's even better, I thought that they were needed by subclasses.
There was a problem hiding this comment.
Nit: we typically write it like dynamicJaasConfig = true.
rajinisivaram
force-pushed
the
KAFKA-4568
branch
from
4f6214b to
90db5d4
Compare
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
LGTM, will merge after Jenkins gives the green light.
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
Remove workaround for testing multiple SASL mechanisms using sasl.jaas.config and the new support for multiple client modules within a JVM. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Sriharsha Chintalapani <harsha@hortonworks.com>, Ismael Juma <ismael@juma.me.uk> Closes #2373 from rajinisivaram/KAFKA-4568 (cherry picked from commit 4c49297) Signed-off-by: Ismael Juma <ismael@juma.me.uk>
|
Refer to this link for build results (access rights to CI server needed): |
Remove workaround for testing multiple SASL mechanisms using sasl.jaas.config and the new support for multiple client modules within a JVM. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Sriharsha Chintalapani <harsha@hortonworks.com>, Ismael Juma <ismael@juma.me.uk> Closes apache#2373 from rajinisivaram/KAFKA-4568
4 participants
Remove workaround for testing multiple SASL mechanisms using sasl.jaas.config and the new support for multiple client modules within a JVM.