You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/06/24 17:23:25 UTC
[GitHub] [kafka] jonathan-albrecht-ibm opened a new pull request, #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
jonathan-albrecht-ibm opened a new pull request, #12343:
URL: https://github.com/apache/kafka/pull/12343
The IBM Semeru JDK use the OpenJDK security providers instead of the IBM
security providers so test for the OpenJDK classes first where possible
and test for Semeru in the java.runtime.name system property otherwise.
Use a real but empty KafkaThread object in KafkaProducerTest instead of
a mock because the Semeru Thread class implementation has some extra
checks that are called and fail when the mock is used.
Tested with IBM and Temurin JDKs. Added a unit test for the new Java.isIbmSemeru() method.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1039696317
##########
clients/src/test/java/org/apache/kafka/common/utils/JavaTest.java:
##########
@@ -46,9 +49,22 @@ public void testIsIBMJdk() {
assertTrue(Java.isIbmJdk());
}
+ @Test
+ public void testIsIBMJdkSemeru() {
+ System.setProperty("java.vendor", "Oracle Corporation");
+ assertFalse(Java.isIbmJdkSemeru());
+ System.setProperty("java.vendor", "IBM Corporation");
+ System.setProperty("java.runtime.name", "Java(TM) SE Runtime Environment");
+ assertFalse(Java.isIbmJdkSemeru());
+ System.setProperty("java.vendor", "IBM Corporation");
+ System.setProperty("java.runtime.name", "IBM Semeru Runtime Certified Edition");
+ assertTrue(Java.isIbmJdkSemeru());
Review Comment:
The original property values are saved and restored in the `before()` and `after()` methods (annotated with `@BeforeEach` and `@AfterEach`) so I think each test method will start with the JVM's property values. Let me know if I'm missing something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1031785699
##########
core/src/test/scala/kafka/security/minikdc/MiniKdc.scala:
##########
@@ -260,11 +260,17 @@ class MiniKdc(config: Properties, workDir: File) extends Logging {
}
private def refreshJvmKerberosConfig(): Unit = {
- val klass =
- if (Java.isIbmJdk)
- Class.forName("com.ibm.security.krb5.internal.Config")
- else
- Class.forName("sun.security.krb5.Config")
+ // Newer IBM JDKs use the OpenJDK security providers so try that first
+ val klass = try {
+ Class.forName("sun.security.krb5.Config")
+ } catch {
+ case ex: Exception => {
+ if (Java.isIbmJdk)
+ Class.forName("com.ibm.security.krb5.internal.Config")
+ else
+ throw ex
+ }
+ }
Review Comment:
Done
##########
core/src/test/scala/unit/kafka/utils/JaasTestUtils.scala:
##########
@@ -119,6 +120,18 @@ object JaasTestUtils {
}
}
+ private val isIbmSecurity = try {
+ Class.forName("sun.security.krb5.Config"); false
+ } catch {
+ case ex: Exception => {
+ if (Java.isIbmJdk) {
+ Class.forName("com.ibm.security.krb5.internal.Config"); true
+ } else {
+ throw ex
+ }
+ }
+ }
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1031785630
##########
clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java:
##########
@@ -2263,7 +2263,10 @@ public void configure(Map<String, ?> configs) {
private Sender sender = mock(Sender.class);
private TransactionManager transactionManager = mock(TransactionManager.class);
private Partitioner partitioner = mock(Partitioner.class);
- private KafkaThread ioThread = mock(KafkaThread.class);
+ private KafkaThread ioThread = new KafkaThread("Fake Kafka Producer I/O Thread", new Runnable() {
+ @Override
+ public void run() {}
+ }, true);
Review Comment:
Done. Amended original commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #12343:
URL: https://github.com/apache/kafka/pull/12343#issuecomment-1167374061
Thanks for the review @Kvicii. The CI seems to have failed due to a server error and looks stuck. Could the build be restarted?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #12343:
URL: https://github.com/apache/kafka/pull/12343#issuecomment-1335553986
Thanks @mimaison!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1039681666
##########
clients/src/main/java/org/apache/kafka/common/utils/Java.java:
##########
@@ -44,6 +44,10 @@ public static boolean isIbmJdk() {
return System.getProperty("java.vendor").contains("IBM");
}
+ public static boolean isIbmJdkSemeru() {
Review Comment:
Instead of having this with the logic in various places, can we not treat IBM Semeru as plain openjdk? If so, we could change the check for IBM JDK to exclude IBM Semeru.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #12343:
URL: https://github.com/apache/kafka/pull/12343#issuecomment-1326790440
TFTR @cadonna, I've updated the changes according to your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1024093773
##########
core/src/test/scala/unit/kafka/utils/JaasTestUtils.scala:
##########
@@ -119,6 +120,18 @@ object JaasTestUtils {
}
}
+ private val isIbmSecurity = try {
+ Class.forName("sun.security.krb5.Config"); false
+ } catch {
+ case ex: Exception => {
+ if (Java.isIbmJdk) {
+ Class.forName("com.ibm.security.krb5.internal.Config"); true
+ } else {
+ throw ex
+ }
+ }
+ }
Review Comment:
See reply above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
mimaison merged PR #12343:
URL: https://github.com/apache/kafka/pull/12343
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #12343:
URL: https://github.com/apache/kafka/pull/12343#issuecomment-1281468382
I have rebased this PR to latest trunk. I don't think the tests that failed are related to these changes. Would it be possible for someone to review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1024086819
##########
clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java:
##########
@@ -2263,7 +2263,10 @@ public void configure(Map<String, ?> configs) {
private Sender sender = mock(Sender.class);
private TransactionManager transactionManager = mock(TransactionManager.class);
private Partitioner partitioner = mock(Partitioner.class);
- private KafkaThread ioThread = mock(KafkaThread.class);
+ private KafkaThread ioThread = new KafkaThread("Fake Kafka Producer I/O Thread", new Runnable() {
+ @Override
+ public void run() {}
+ }, true);
Review Comment:
I have run the test again without this change and it looks like it is no longer needed. I see that mockito has been upgraded to newer releases a few times since this PR was created so I'm guessing that has fixed this problem.
I'll push an amended commit to remove this change but let me know if the kafka developers prefer adding a new commit to remove it instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
cadonna commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1021245518
##########
clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java:
##########
@@ -2263,7 +2263,10 @@ public void configure(Map<String, ?> configs) {
private Sender sender = mock(Sender.class);
private TransactionManager transactionManager = mock(TransactionManager.class);
private Partitioner partitioner = mock(Partitioner.class);
- private KafkaThread ioThread = mock(KafkaThread.class);
+ private KafkaThread ioThread = new KafkaThread("Fake Kafka Producer I/O Thread", new Runnable() {
+ @Override
+ public void run() {}
+ }, true);
Review Comment:
Is it possible to set up the mock in such a way, so that it can be used?
Are there native methods involved? That woulod be a reason to not use a mock since Mockito cannot mock native methods.
##########
core/src/test/scala/kafka/security/minikdc/MiniKdc.scala:
##########
@@ -260,11 +260,17 @@ class MiniKdc(config: Properties, workDir: File) extends Logging {
}
private def refreshJvmKerberosConfig(): Unit = {
- val klass =
- if (Java.isIbmJdk)
- Class.forName("com.ibm.security.krb5.internal.Config")
- else
- Class.forName("sun.security.krb5.Config")
+ // Newer IBM JDKs use the OpenJDK security providers so try that first
+ val klass = try {
+ Class.forName("sun.security.krb5.Config")
+ } catch {
+ case ex: Exception => {
+ if (Java.isIbmJdk)
+ Class.forName("com.ibm.security.krb5.internal.Config")
+ else
+ throw ex
+ }
+ }
Review Comment:
Why do you not use a method `Java.isIbmJdkSemeru` here similar to what you did for the clients? I think that would improve readability and avoid the comment.
##########
core/src/test/scala/unit/kafka/utils/JaasTestUtils.scala:
##########
@@ -119,6 +120,18 @@ object JaasTestUtils {
}
}
+ private val isIbmSecurity = try {
+ Class.forName("sun.security.krb5.Config"); false
+ } catch {
+ case ex: Exception => {
+ if (Java.isIbmJdk) {
+ Class.forName("com.ibm.security.krb5.internal.Config"); true
+ } else {
+ throw ex
+ }
+ }
+ }
Review Comment:
See my comment above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #12343:
URL: https://github.com/apache/kafka/pull/12343#issuecomment-1192747607
Hi @Kvicii, I have rebased this PR to trigger the tests. It looks like everything that ran passed except for 2 instances of
```
Build / JDK 8 and Scala 2.12 / testFencedLeaderRecovery – org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest
```
From a quick look at other PR tests, I think these are just flakey eg. https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-12396/3/pipeline/14
Would it be possible to merge this PR? Let me know if there is anything else I need to do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1024093478
##########
core/src/test/scala/kafka/security/minikdc/MiniKdc.scala:
##########
@@ -260,11 +260,17 @@ class MiniKdc(config: Properties, workDir: File) extends Logging {
}
private def refreshJvmKerberosConfig(): Unit = {
- val klass =
- if (Java.isIbmJdk)
- Class.forName("com.ibm.security.krb5.internal.Config")
- else
- Class.forName("sun.security.krb5.Config")
+ // Newer IBM JDKs use the OpenJDK security providers so try that first
+ val klass = try {
+ Class.forName("sun.security.krb5.Config")
+ } catch {
+ case ex: Exception => {
+ if (Java.isIbmJdk)
+ Class.forName("com.ibm.security.krb5.internal.Config")
+ else
+ throw ex
+ }
+ }
Review Comment:
The idea was to just check for the classes directly but you're definitely right that `Java.isIbmJdkSemeru` is more readable. I have a new commit to make this change and the other one below, but I'll hold off on pushing it to see if any more feedback from @ijuma or @hachikuji.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1039680283
##########
clients/src/test/java/org/apache/kafka/common/utils/JavaTest.java:
##########
@@ -46,9 +49,22 @@ public void testIsIBMJdk() {
assertTrue(Java.isIbmJdk());
}
+ @Test
+ public void testIsIBMJdkSemeru() {
+ System.setProperty("java.vendor", "Oracle Corporation");
+ assertFalse(Java.isIbmJdkSemeru());
+ System.setProperty("java.vendor", "IBM Corporation");
+ System.setProperty("java.runtime.name", "Java(TM) SE Runtime Environment");
+ assertFalse(Java.isIbmJdkSemeru());
+ System.setProperty("java.vendor", "IBM Corporation");
+ System.setProperty("java.runtime.name", "IBM Semeru Runtime Certified Edition");
+ assertTrue(Java.isIbmJdkSemeru());
Review Comment:
Looks like we don't clear the set property, won't that pollute the rest of the test suite that runs in that jvm?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1039711882
##########
clients/src/test/java/org/apache/kafka/common/utils/JavaTest.java:
##########
@@ -46,9 +49,22 @@ public void testIsIBMJdk() {
assertTrue(Java.isIbmJdk());
}
+ @Test
+ public void testIsIBMJdkSemeru() {
+ System.setProperty("java.vendor", "Oracle Corporation");
+ assertFalse(Java.isIbmJdkSemeru());
+ System.setProperty("java.vendor", "IBM Corporation");
+ System.setProperty("java.runtime.name", "Java(TM) SE Runtime Environment");
+ assertFalse(Java.isIbmJdkSemeru());
+ System.setProperty("java.vendor", "IBM Corporation");
+ System.setProperty("java.runtime.name", "IBM Semeru Runtime Certified Edition");
+ assertTrue(Java.isIbmJdkSemeru());
Review Comment:
Thanks for the quick response. Yes, you're right, I had missed that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1039681666
##########
clients/src/main/java/org/apache/kafka/common/utils/Java.java:
##########
@@ -44,6 +44,10 @@ public static boolean isIbmJdk() {
return System.getProperty("java.vendor").contains("IBM");
}
+ public static boolean isIbmJdkSemeru() {
Review Comment:
Instead of having this with the logic in various places, can we not stream IBM Semeru is plain openjdk? If so, we could change the check for IBM JDK to exclude IBM Semeru.
##########
clients/src/main/java/org/apache/kafka/common/utils/Java.java:
##########
@@ -44,6 +44,10 @@ public static boolean isIbmJdk() {
return System.getProperty("java.vendor").contains("IBM");
}
+ public static boolean isIbmJdkSemeru() {
Review Comment:
Instead of having this with the logic in various places, can we not stream IBM Semeru as plain openjdk? If so, we could change the check for IBM JDK to exclude IBM Semeru.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jonathan-albrecht-ibm commented on a diff in pull request #12343: MINOR: Update unit/integration tests to work with the IBM Semeru JDK
Posted by GitBox <gi...@apache.org>.
jonathan-albrecht-ibm commented on code in PR #12343:
URL: https://github.com/apache/kafka/pull/12343#discussion_r1039745728
##########
clients/src/main/java/org/apache/kafka/common/utils/Java.java:
##########
@@ -44,6 +44,10 @@ public static boolean isIbmJdk() {
return System.getProperty("java.vendor").contains("IBM");
}
+ public static boolean isIbmJdkSemeru() {
Review Comment:
I don't know if we can treat the IBM Semeru JDK as openjdk in all places where `isIbmJdk()` is being used. The changes here fix the places where the IBM security classes were being incorrectly specified for Semeru. I could look at the other places but since there aren't tests that are failing or obvious regression, I would need to investigate more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org