MINOR: buffer should ignore caching

[vvcephei]

When the buffer size config is set to 0, Streams invokes withCachingDisabled in all registered stores.

Previously, we didn't expect this method to be called on the suppression buffer, but since it can be under valid circumstances, we should just ignore it rather than throwing an exception.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[vvcephei]

@mjsax @bbejeck

I ran into this while testing the new suppression feature. I think it should be considered a 2.1 bug.

[bbejeck]

Thanks, @vvcephei I agree with the change, looks good to me. Can we also have a unit test to verify the behavior?

[bbejeck]

Should we log a warning here and below?

[vvcephei]

I'm not sure I agree with a warning.

I think it's fine to just ignore the call, since the buffer will behave correctly whether you call these methods or not. In fact, I originally had them no-op, and changed it to throw upon review.

There's no publicly facing API to register a buffer, so there's no way that a user could call these methods right now. In fact, the buffer itself is an internal class. Therefore, the only way these methods get called are via Streams itself.

My thought is that it would be confusing to see warnings that actually indicate nothing is wrong and about which you can do nothing.

Would you be satisfied with a javadoc explaining the decision?

[bbejeck]

Yeah, my intent was just some sort of notification to users these methods are ignored, javadoc is fine with me.

[mjsax]

Can you redirect the PR against trunk?

@lindong28 Can we get this into 2.1 before you cut the first RC?

[mjsax]

We should add JavaDoc and point out that he method can be called, but won't do anything, as caching is not supported. Similar below.

[vvcephei]

done.

[lindong28]

@mjsax Certainly. I am still waiting for the system test branch to be created for 2.1 branch in https://jenkins.confluent.io/job/system-test-kafka before cutting the first RC. Please feel free to cherry-pick into 2.1 branch when it is ready.

[vvcephei]

Thanks @lindong28. We'll let you know when it's cherry-picked.

Thanks @bbejeck and @mjsax for the reviews. I've rebased on trunk and addressed your comments.

[vvcephei]

java 8 failures:
kafka.server.DynamicBrokerReconfigurationTest.testAddRemoveSaslListeners
java 11 failures:

org.apache.kafka.clients.producer.internals.BufferPoolTest > testBlockTimeout FAILED
> Task :clients:unitTest FAILED
kafka.server.DynamicBrokerReconfigurationTest > testAddRemoveSaslListeners FAILED
kafka.server.DynamicBrokerReconfigurationTest > testUncleanLeaderElectionEnable FAILED
> Task :core:integrationTest FAILED
org.apache.kafka.streams.integration.EosIntegrationTest > shouldNotViolateEosIfOneTaskFails FAILED
> Task :streams:integrationTest FAILED

Retest this, please.

[bbejeck]

LGTM

[mjsax]

Java11 failed again with... @vvcephei Can you investigate?

java.lang.AssertionError: 
Expected: <[KeyValue(0, 0), KeyValue(0, 1), KeyValue(0, 2), KeyValue(0, 3), KeyValue(0, 4), KeyValue(0, 5), KeyValue(0, 6), KeyValue(0, 7), KeyValue(0, 8), KeyValue(0, 9)]>
     but: was <[KeyValue(0, 0), KeyValue(0, 1), KeyValue(0, 2), KeyValue(0, 3), KeyValue(0, 4), KeyValue(0, 5), KeyValue(0, 6), KeyValue(0, 7), KeyValue(0, 8), KeyValue(0, 9), KeyValue(0, 10), KeyValue(0, 11), KeyValue(0, 12), KeyValue(0, 13), KeyValue(0, 14)]>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
	at org.apache.kafka.streams.integration.EosIntegrationTest.checkResultPerKey(EosIntegrationTest.java:218)
	at org.apache.kafka.streams.integration.EosIntegrationTest.shouldNotViolateEosIfOneTaskGetsFencedUsingIsolatedAppInstances(EosIntegrationTest.java:520)

[mjsax]

Retest this please

[vvcephei]

@mjsax Can we make a Jira for it instead? I really don't think that this change could be linked with the EosIntegrationTest.

[vvcephei]

Hah! Vindication: this time only kafka.log.LogCleanerIntegrationTest.testMarksPartitionsAsOfflineAndPopulatesUncleanableMetrics failed.

Geez. This java11 situation is not cool...

[mjsax]

One more try... I we can get green Java11, I'll merge afterwards. (I agree that the change should be unrelated to EOS test -- still, we should try to get a green build. @bbejeck is looking into the problem already -- we can still create a JIRA if there is no quick fix -- leave it to @bbejeck to make a call).

Retest this please.

[mjsax]

Merged to trunk and cherry-picked to 2.1 branch. (\cc @lindong28 -- thanks a lot!)

[vvcephei]

Thanks @mjsax, @bbejeck , and @lindong28 !