MINOR: Improve usage of LogCaptureAppender #8508

mjsax · 2020-04-18T00:29:02Z

I started a "small" cleanup, and it ended up to become a "big" cleanup...

limit the usage of LogCaptureAppender to the class under test (if possible)
ensure that the registered appended is unregistered (via try-with-resource)
limit the "scope" of creating/unregistering the appended to a minimum
cleanup all warning of all classes touched
some code reformatting
some class renaming

Call for review @vvcephei

mjsax · 2020-04-18T00:31:40Z

...ams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKTableJoinProcessor.java

@@ -30,7 +30,7 @@
 import static org.apache.kafka.streams.state.ValueAndTimestamp.getValueOrNull;

 class KStreamKTableJoinProcessor<K1, K2, V1, V2, R> extends AbstractProcessor<K1, V1> {
-    private static final Logger LOG = LoggerFactory.getLogger(KStreamKTableJoinProcessor.class);
+    private static final Logger LOG = LoggerFactory.getLogger(KStreamKTableJoin.class);


This is an actual change -- in all other processor, we use the "top level class" for the logger and not the Processor class -- changing this for alignment.

mjsax · 2020-04-18T00:32:13Z

streams/src/test/java/org/apache/kafka/streams/kstream/internals/KGroupedStreamImplTest.java

@@ -659,7 +657,6 @@ public void shouldCountWindowed() {
    @Test
    public void shouldCountWindowedWithInternalStoreName() {
        final MockProcessorSupplier<Windowed<String>, Long> supplier = new MockProcessorSupplier<>();
-        final List<KeyValue<Windowed<String>, KeyValue<Long, Long>>> results = new ArrayList<>();


Removed because unused.

mjsax · 2020-04-18T00:32:29Z

streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoinTest.java

@@ -186,15 +189,12 @@ public void shouldThrowExceptionWhenOtherJoinStoreSetsRetainDuplicatesFalse() {
    @Test
    public void shouldBuildJoinWithCustomStoresAndCorrectWindowSettings() {
        //Case where everything matches up
-        final WindowBytesStoreSupplier thisStoreSupplier = buildWindowBytesStoreSupplier("in-memory-join-store", 150, 100, true);
-        final WindowBytesStoreSupplier otherStoreSupplier = buildWindowBytesStoreSupplier("in-memory-join-store-other", 150, 100, true);


Removed because unused.

mjsax · 2020-04-18T00:34:46Z

...ams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamWindowAggregateTest.java

-            processors.get(1).checkAndClearProcessResult(new KeyValueTimestamp[0]);
-            processors.get(2).checkAndClearProcessResult(new KeyValueTimestamp[0]);
+            processors.get(1).checkAndClearProcessResult();
+            processors.get(2).checkAndClearProcessResult();


Removed because the parameter is variadic

mjsax · 2020-04-18T00:36:40Z

streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamThreadTest.java


-    private void verifyLogMessagesSkippedRecordsForDeserializationException(final LogCaptureAppender appender) {


This method is only called once and thus its merged into the main test method

mjsax · 2020-04-18T00:38:07Z

...ms/src/test/java/org/apache/kafka/streams/state/internals/AbstractSessionBytesStoreTest.java

@@ -80,8 +81,6 @@

    abstract String getMetricsScope();

-    abstract void setClassLoggerToDebug();


We don't capture any debug logs and thus it's getting removed

mjsax · 2020-04-18T00:41:38Z

streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamThreadTest.java


-    private void verifyLogMessagesForSkippedRecordsForInvalidTimestamps(final LogCaptureAppender appender) {


This method is only called once and thus its merged into the main test method

ijuma · 2020-04-18T15:46:45Z

Did you consider using try with resources?

vvcephei

Thanks, @mjsax ! This is awesome. I'm wondering if we can formalize the pattern you've adopted by implementing AutoCloseable in LogCaptureAppender, and making the close method unregister the appender, so we can convert all those try/finally blocks to try-with-resources? Of course, we'd have to move the assertions inside the block, which we have sometimes and not other times, but it doesn't seem like a big deal either way.

vvcephei · 2020-04-18T19:33:50Z

streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java

@@ -801,8 +801,11 @@ public void shouldWarnAboutRocksDBConfigSetterIsNotGuaranteedToBeBackwardsCompat
        props.setProperty(StreamsConfig.ROCKSDB_CONFIG_SETTER_CLASS_CONFIG, TestRocksDbConfigSetter.class.getName());

        final LogCaptureAppender appender = LogCaptureAppender.createAndRegister();


Seems like this appender could also be scoped to the class that logs the message we want to capture, right?

Unfortunately not; I tied it and it fails. The issue is, we would want to register the LogCaptureAppender only for KafkaStreams.class but at this point in the test, the corresponding logger does not exist yet: it's only created in the constructor call, that we actually test. Does this make sense?

vvcephei

Thanks, @mjsax ! It seems like we should look into the try-with-resources thing, unless you already rejected that option for some reason. I also registered one complaint below.

But I don't want to block on either one, if you really prefer the current code.

Other than those two issues, it's a really nice cleanup. Thanks!

vvcephei · 2020-04-18T19:49:50Z

...st/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSegmentedBytesStoreTest.java

@@ -512,19 +512,24 @@ private Bytes serializeKey(final Windowed<String> key) {
        final StateSerdes<String, Long> stateSerdes = StateSerdes.withBuiltinTypes("dummy", String.class, Long.class);
        while (iterator.hasNext()) {
            final KeyValue<Bytes, byte[]> next = iterator.next();
+            final KeyValue<Windowed<String>, Long> deserialized;


Not a huge fan of this particular "clean up". We're trading a tightly defined scope for de-duplicating the trivial line results.add(deserialization). It doesn't seem like a good trade to me.

I don't feel "strong" about it -- it was an IntelliJ warning/suggestion to "share common code".

mjsax · 2020-04-19T08:16:24Z

I did not consider using try-with-resource. Great idea!

mjsax · 2020-04-19T08:52:40Z

Updated this. Call for review.

vvcephei

Thanks, @mjsax ; this is awesome!

I just had one question about renaming the test thread. Otherwise, it LGTM!

vvcephei · 2020-04-20T20:24:26Z

...ams/src/test/java/org/apache/kafka/streams/processor/internals/InternalTopicManagerTest.java

+        // When executing on Jenkins, the thread name is set to an unknown value,
+        // hence, we need to set it explicitly to make our log-assertions pass
+        Thread.currentThread().setName(threadName);


Hmm, this might be surprising. Won't this cause the test executor thread to be called "threadName" from now until the end of the build?

Do you think we could instead get the currentThread's name and use that in the assertions? Or otherwise make the assertions agnostic to the name of the thread?

Getting the name is much better! Should have done this for the beginning on...

mjsax · 2020-04-20T20:59:58Z

Java 8 passed.
Java11: kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup
Java14 failed due to #8519 (already merged).

Retest this please.

mjsax · 2020-04-20T21:48:08Z

Updated this PR. Will merge after Jenkins passe.

vvcephei

LGTM! Thanks.

mjsax · 2020-04-21T03:45:15Z

Java 8: org.apache.kafka.streams.integration.StandbyTaskEOSIntegrationTest.surviveWithOneTaskAsStandby[exactly_once_beta]
Java 11: org.apache.kafka.connect.mirror.MirrorConnectorsIntegrationTest.testReplication
Java 14: org.apache.kafka.streams.integration.StoreQueryIntegrationTest.shouldQuerySpecificActivePartitionStores

Retest this please.

mjsax · 2020-04-21T16:23:35Z

Java 8 and Java 11 passed.
Java 14: failed with unknown error...

mjsax force-pushed the minor-imporve-log-capturing branch from 422aae9 to ccab82c Compare April 18, 2020 00:30

mjsax commented Apr 18, 2020

View reviewed changes

vvcephei reviewed Apr 18, 2020

View reviewed changes

vvcephei approved these changes Apr 18, 2020

View reviewed changes

mjsax force-pushed the minor-imporve-log-capturing branch from 8a9d0aa to 81d042b Compare April 19, 2020 08:47

MINOR: Improve usage of LogCaptureAppender

4b65333

mjsax force-pushed the minor-imporve-log-capturing branch from b594d46 to 4b65333 Compare April 20, 2020 18:17

vvcephei reviewed Apr 20, 2020

View reviewed changes

Github comments

8e72cdd

vvcephei approved these changes Apr 20, 2020

View reviewed changes

mjsax merged commit 11d8ef7 into apache:trunk Apr 21, 2020

mjsax deleted the minor-imporve-log-capturing branch April 21, 2020 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: Improve usage of LogCaptureAppender #8508

MINOR: Improve usage of LogCaptureAppender #8508

mjsax commented Apr 18, 2020 •

edited

mjsax Apr 18, 2020

mjsax Apr 18, 2020

mjsax Apr 18, 2020

mjsax Apr 18, 2020

mjsax Apr 18, 2020

mjsax Apr 18, 2020

mjsax Apr 18, 2020

ijuma commented Apr 18, 2020

vvcephei left a comment

vvcephei Apr 18, 2020

mjsax Apr 19, 2020

vvcephei left a comment

vvcephei Apr 18, 2020

mjsax Apr 19, 2020

mjsax commented Apr 19, 2020

mjsax commented Apr 19, 2020

vvcephei left a comment

vvcephei Apr 20, 2020

mjsax Apr 20, 2020

mjsax commented Apr 20, 2020

mjsax commented Apr 20, 2020

vvcephei left a comment

mjsax commented Apr 21, 2020

mjsax commented Apr 21, 2020


		private void verifyLogMessagesSkippedRecordsForDeserializationException(final LogCaptureAppender appender) {

		@@ -80,8 +81,6 @@

		abstract String getMetricsScope();

		abstract void setClassLoggerToDebug();


		private void verifyLogMessagesForSkippedRecordsForInvalidTimestamps(final LogCaptureAppender appender) {

		@@ -801,8 +801,11 @@ public void shouldWarnAboutRocksDBConfigSetterIsNotGuaranteedToBeBackwardsCompat
		props.setProperty(StreamsConfig.ROCKSDB_CONFIG_SETTER_CLASS_CONFIG, TestRocksDbConfigSetter.class.getName());

		final LogCaptureAppender appender = LogCaptureAppender.createAndRegister();

MINOR: Improve usage of LogCaptureAppender #8508

MINOR: Improve usage of LogCaptureAppender #8508

Conversation

mjsax commented Apr 18, 2020 • edited

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

ijuma commented Apr 18, 2020

vvcephei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvcephei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax commented Apr 19, 2020

mjsax commented Apr 19, 2020

vvcephei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax commented Apr 20, 2020

mjsax commented Apr 20, 2020

vvcephei left a comment

Choose a reason for hiding this comment

mjsax commented Apr 21, 2020

mjsax commented Apr 21, 2020

mjsax commented Apr 18, 2020 •

edited