-
Notifications
You must be signed in to change notification settings - Fork 14k
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-4501: Java 9 compilation and runtime fixes #3647
Conversation
Refer to this link for build results (access rights to CI server needed): |
retest this please |
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): |
1fca2f0
to
2e92b1d
Compare
Refer to this link for build results (access rights to CI server needed): |
Review by @hachikuji. |
Refer to this link for build results (access rights to CI server needed): |
57ce4e9
to
910a050
Compare
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): |
@hachikuji, https://builds.apache.org/job/kafka-trunk-jdk9/1/console shows that the build compiles with Java 9, but EasyMock/PowerMock tests fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems straightforward. Just a couple questions.
@@ -269,8 +270,8 @@ object LogConfig { | |||
*/ | |||
def fromProps(defaults: java.util.Map[_ <: Object, _ <: Object], overrides: Properties): LogConfig = { | |||
val props = new Properties() | |||
props.putAll(defaults) | |||
props.putAll(overrides) | |||
defaults.asScala.foreach { case (k, v) => props.put(k, v) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why you didn't add another implicit ++=
for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key type is Object instead of String, which is unsafe. We should probably change the type of the map received by LogConfig but I didn't want to tackle that here.
protected ScramSaslClientProvider() { | ||
super("SASL/SCRAM Client Provider", 1.0, "SASL/SCRAM Client Provider for Kafka"); | ||
for (ScramMechanism mechanism : ScramMechanism.values()) | ||
super.put("SaslClientFactory." + mechanism.mechanismName(), ScramSaslClientFactory.class.getName()); | ||
put("SaslClientFactory." + mechanism.mechanismName(), ScramSaslClientFactory.class.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The super qualifier didn't achieve anything.
} | ||
URL_ENCODE_NO_PADDING = encode.bindTo(juUrlEncoderNoPassing); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we need the extra newlines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unintentional, will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will let you merge when you fix the unneeded newlines.
910a050
to
e65323a
Compare
Fixed the empty lines, merging to trunk. |
Compilation error fixes:
code (Ambiguous reference when invoking Properties.putAll() in Java 11 scala/bug#10418)
Ambiguous reference when invoking Properties.putAll() in Java 11 scala/bug#10418 (comment))
Java 9 support findbugsproject/findbugs#105)
Compilation warning fixes:
Runtime error fixes:
Also:
Note that tests involving EasyMock (easymock/easymock#193)
or PowerMock (powermock/powermock#783)
will fail as neither supports Java 9 currently.