abolish scala. make test harnesses java, avoid any link-time dependency on scala #65

radai-rosenblatt · 2017-09-20T17:23:48Z

No description provided.

…cy on scala

smccauliff · 2017-09-27T18:22:15Z

src/main/java/com/linkedin/kafka/clients/producer/UUIDFactory.java

@@ -17,12 +17,13 @@
 public interface UUIDFactory<K, V> extends Configurable {

  /**
-   * Get a random UUID.
+   * @return a random UUID


If you are cleaning up documentation then you might as well change this to something like "a non-null UUID". The UUID does not need to be random.

smccauliff · 2017-09-27T18:23:23Z

src/test/java/com/linkedin/kafka/clients/consumer/LiKafkaConsumerIntegrationTest.java

@@ -1052,6 +1055,14 @@ private void produceSyntheticMessages(String topic) {
    producer.close();
  }

+  //TODO - remove / refactor
+  private Producer<byte[], byte[]> createKafkaProducer() {


Should we default all tests to SSL?

no. this method is rather hacky and creates an ad-hoc producer<byte[], byte[]> where the rest of the harness code creates <string, string>, and so should be cleaned up somehow.
however, our tests currently rely on this heavily nd i didnt want ot make this PR any bogger.

smccauliff · 2017-09-27T18:25:14Z

.../java/com/linkedin/kafka/clients/utils/tests/AbstractKafkaClientsIntegrationTestHarness.java

+  @Override
+  public void tearDown() {
+    try {
+      //TODO - something here ?


smccauliff · 2017-09-27T18:30:21Z

.../java/com/linkedin/kafka/clients/utils/tests/AbstractKafkaClientsIntegrationTestHarness.java

+
+    //apply overrides
+    if (overrides != null) {
+      overrides.forEach(result::put);


result.putAll(overrides)?

smccauliff · 2017-09-27T18:36:51Z

src/test/java/com/linkedin/kafka/clients/utils/tests/AbstractKafkaIntegrationTestHarness.java

+  }
+
+
+  protected List<Map<Object, Object>> buildBrokerConfigs() {


It might be useful to add documentation about this return value so the user knows that returnValue.get(i) is a config for the ith broker.

smccauliff · 2017-09-27T18:39:27Z

src/test/java/com/linkedin/kafka/clients/utils/tests/AbstractZookeeperTestHarness.java

+package com.linkedin.kafka.clients.utils.tests;
+
+public abstract class AbstractZookeeperTestHarness {
+  protected EmbeddedZookeeper zookeeper = null;


zookeeper -> _zookeeper

becketqin · 2017-09-27T18:14:49Z

src/test/resources/log4j.properties

@@ -2,7 +2,7 @@
 # Copyright 2017 LinkedIn Corp. Licensed under the BSD 2-Clause License (the "License").  See License in the project root for license information.
 #

-log4j.rootLogger=OFF, stdout
+log4j.rootLogger=DEBUG, stdout


Accidental change?

becketqin · 2017-09-27T19:25:27Z

src/test/java/com/linkedin/kafka/clients/utils/tests/KafkaTestUtilsTest.java

@@ -0,0 +1,26 @@
+/*


This test is really weird....

becketqin · 2017-09-27T19:28:58Z

src/test/java/com/linkedin/kafka/clients/largemessage/LargeMessageIntegrationTest.java

@@ -50,7 +47,7 @@ public int clusterSize() {
  @Override
  public Properties overridingProps() {
    Properties props = new Properties();
-    props.setProperty(KafkaConfig.NumPartitionsProp(), Integer.toString(NUM_PARTITIONS));
+    props.setProperty("num.partitions", Integer.toString(NUM_PARTITIONS));


It seems safer to use the method instead of hard coding it.

that prop is in scala code. it is unclean ...

becketqin · 2017-09-28T00:19:07Z

src/test/java/com/linkedin/kafka/clients/utils/tests/KafkaTestUtils.java

+        try {
+          FileUtils.forceDelete(toCleanUp);
+        } catch (IOException issue) {
+          issue.printStackTrace(System.err);


Maybe rethrow the exception to let the uncaught exception handler handle it.

becketqin · 2017-09-28T00:27:34Z

src/test/java/com/linkedin/kafka/clients/utils/tests/KafkaTestUtils.java

+    return consumer;
+  }
+
+  public static int getAvailableTcpPort() {


Can we reuse the KafkaServer.boundPort() method and ZookeeperServer.getClientPort() instead of writing our own method?

smccauliff · 2017-09-28T01:02:09Z

src/test/java/com/linkedin/kafka/clients/utils/tests/AbstractKafkaIntegrationTestHarness.java

+        }
+      } else {
+        if (trustStoreFile() != null) {
+          throw new AssertionError("security protocol not yet yet trust store file provided");


I don't understand this error message.

smccauliff · 2017-09-28T01:07:34Z

src/test/java/com/linkedin/kafka/clients/utils/tests/EmbeddedBroker.java

+  private final static Method SHUTDOWN_METHOD;
+  private final static Method AWAIT_SHUTDOWN_METHOD;
+  private final static Object EMPTY_OPTION;
+  private final static Pattern LISTENER_PATTERN = Pattern.compile("\\s*(\\w+)://([\\w\\d]+):(\\d+)\\s*");


This won't match IPv6 addresses which someone might want to use if they were testing that IPv6 worked.

See https://www.ietf.org/rfc/rfc2732.txt

Probably the \s* at the beginning and end is not needed if matcher.find() is used.

smccauliff · 2017-09-28T01:21:59Z

src/test/java/com/linkedin/kafka/clients/utils/tests/EmbeddedBrokerBuilder.java

+    return this;
+  }
+
+  public EmbeddedBrokerBuilder trustStore(File trustStore) {


Do the trust stores need to be files or can they be resources so that you can package truststores in the source code.

the scala harness uses a File as a trust store, and i matched it.
also, this is test code, i see no use case for packaging trust stores into the artifacts ?

smccauliff · 2017-09-28T01:26:06Z

src/test/java/com/linkedin/kafka/clients/utils/tests/KafkaTestUtils.java

+  private final static List<File> FILES_TO_CLEAN_UP = Collections.synchronizedList(new ArrayList<>());
+
+  static {
+    SHUTDOWN_HOOK = new Thread(() -> {


Is this just here just in case tear down does not work?

no. closable test classes (like embeddable brokers and zk) should delete their files on close, but other tests may need files/folders

becketqin

LGTM.

radai-rosenblatt force-pushed the scala-be-gone branch from 3cfb079 to ad23b2c Compare September 27, 2017 18:13

abolish scala. make test harnesses java, avoid any link-time dependen…

a62227e

…cy on scala

radai-rosenblatt force-pushed the scala-be-gone branch from ad23b2c to a62227e Compare September 27, 2017 18:17

smccauliff reviewed Sep 27, 2017

View reviewed changes

address review comments, disable flaky test on OSX

527daec

radai-rosenblatt force-pushed the scala-be-gone branch from f8054e7 to 527daec Compare September 27, 2017 22:12

becketqin reviewed Sep 28, 2017

View reviewed changes

smccauliff reviewed Sep 28, 2017

View reviewed changes

address more comments

a573792

becketqin approved these changes Sep 28, 2017

View reviewed changes

radai-rosenblatt merged commit a473282 into linkedin:master Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

abolish scala. make test harnesses java, avoid any link-time dependency on scala #65

abolish scala. make test harnesses java, avoid any link-time dependency on scala #65

radai-rosenblatt commented Sep 20, 2017

smccauliff Sep 27, 2017

smccauliff Sep 27, 2017

radai-rosenblatt Sep 27, 2017

smccauliff Sep 27, 2017

smccauliff Sep 27, 2017

smccauliff Sep 27, 2017

smccauliff Sep 27, 2017

becketqin Sep 27, 2017

becketqin Sep 27, 2017

becketqin Sep 27, 2017

radai-rosenblatt Sep 28, 2017

becketqin Sep 28, 2017

becketqin Sep 28, 2017

smccauliff Sep 28, 2017

smccauliff Sep 28, 2017

smccauliff Sep 28, 2017

smccauliff Sep 28, 2017 •

edited

Loading

smccauliff Sep 28, 2017

radai-rosenblatt Sep 28, 2017

smccauliff Sep 28, 2017

radai-rosenblatt Sep 28, 2017

becketqin left a comment

abolish scala. make test harnesses java, avoid any link-time dependency on scala #65

abolish scala. make test harnesses java, avoid any link-time dependency on scala #65

Conversation

radai-rosenblatt commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smccauliff Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becketqin left a comment

Choose a reason for hiding this comment

smccauliff Sep 28, 2017 •

edited

Loading