[KAFKA-7379] [streams] send.buffer.bytes should be allowed to set -1 in KafkaStreams by aai95 · Pull Request #5643 · apache/kafka

aai95 · 2018-09-12T13:58:19Z

What changes were proposed in this pull request?

atLeast(0) in StreamsConfig, ProducerConfig and ConsumerConfig were replaced by SEND_BUFFER_LOWER_BOUND and RECEIVE_BUFFER_LOWER_BOUND from CommonClientConfigs.

How was this patch tested?

Three unit tests were added to KafkaStreamsTest

Committer Checklist (excluded from commit message)

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

aai95 · 2018-09-12T14:00:00Z

@mjsax review my changes, please.
Is my understanding of the goal of this JIRA correct?

vvcephei · 2018-09-12T16:36:23Z

Hey @aai95 ,
Thanks for the PR!

I think we only wanted to change the bound on send and receive buffers.

Also, can you think of some way to test this change?

Thanks,
-John

aai95 · 2018-09-13T13:12:15Z

Hi @vvcephei,
Thank you for your remarks! I made corresponding changes.
Regarding test coverage, as I understand org.apache.kafka.clients.producer.KafkaProducerTest and
org.apache.kafka.clients.consumer.KafkaConsumerTest already contain methods testInvalidSocketSendBufferSize and testInvalidSocketReceiveBufferSize
which check these cases if send.buffer.bytes and receive.buffer.bytes become less than -1.

mjsax · 2018-09-13T17:36:04Z

@aai95 trunk branch was broken recently -- can you please rebase this PR to pick up latest trunk that is fixed now? Otherwise, we can't get a green build. Thx.

For testing, we could add a test, but if Consumer/Producer would change the bounds, our test would not catch this anyway. Adding those parameters is basically code duplication.

If we want to improve the code, Consumer/Producer (or mabye CommonConfig) should define a public final static variable, that define the lower bound, and we could use this in all three classes. This way, if the bounds change, all are updated and we don't miss anything.

Thoughts?

aai95 · 2018-09-14T13:00:56Z

I performed rebase of my PR just to check that build passes. The test will be added.

aai95 · 2018-09-18T12:58:19Z

@mjsax review my changes, please.
I implemented your suggestion regarding global constants. Thanks.
In classes KafkaProducerTest and KafkaConsumerTest we have method testOsDefaultSocketBufferSizes.
Here value Selectable.USE_DEFAULT_BUFFER_SIZE is -1, as I understand this case covers my changes.
This test successfully passed before, because in ProducerConfig and ConsumerConfig we had hard-coded value -1, now we have global constant in CommonClientConfigs.

mjsax · 2018-09-18T21:11:35Z

@guozhangwang @hachikuji I think we can do this without a KIP, but I would like to very with you.

sounds good to me.

guozhangwang · 2018-09-19T22:15:26Z

@aai95 could you add the unit test in KafkaStreamsTest for testing the scope of this config for Streams as well? The existing tests in KafkaProducerTest / KafkaConsumerTest does not cover streams case.

…pache#7379)

aai95 · 2018-09-20T12:50:11Z

@guozhangwang review my changes, please.
Three unit tests were added to KafkaStreamsTest

guozhangwang

LGTM!

guozhangwang · 2018-09-20T19:19:09Z

The jenkins failure is not relevant.

mjsax

Just catching up on this PR -- couple of follow up comments -- maybe worth to do a follow up PR?

One more meta comment: why did we add the test in KafkaStreamsTest but not in StreamsConfigTest? Seems to be miss placed.

mjsax · 2018-09-21T04:19:50Z

+        props.put(CommonClientConfigs.SEND_BUFFER_CONFIG, Selectable.USE_DEFAULT_BUFFER_SIZE);
+        props.put(CommonClientConfigs.RECEIVE_BUFFER_CONFIG, Selectable.USE_DEFAULT_BUFFER_SIZE);
+        final KafkaStreams streams = new KafkaStreams(builder.build(), props);
+        streams.close();


Just catching up on this PR. Should we wrap the close() into a finally block?

mjsax · 2018-09-21T04:25:21Z

    }

+    @Test
+    public void testOsDefaultSocketBufferSizes() {


nit: naming -> shouldAcceptDefaultBufferSizes()

Also, I am wondering why we check for default buffer size? The ticket was about the issue, that -1 was not accepted. Thus, while having this test is ok, we should actually test for -1 to have a test that covers the reported issue.

mjsax · 2018-09-21T04:25:38Z

+    public void testInvalidSocketSendBufferSize() {
+        props.put(CommonClientConfigs.SEND_BUFFER_CONFIG, -2);
+        final KafkaStreams streams = new KafkaStreams(builder.build(), props);
+        streams.close();


If new KafkaStreams throws, this line will never be executed (same below). Also, streams will not have any object assigned. Thus, we cannot even close anything, as the object was never created -- I think we can remove this line and also remove the variable streams.

mjsax · 2018-09-21T04:26:08Z

+    }
+
+    @Test(expected = KafkaException.class)
+    public void testInvalidSocketSendBufferSize() {


nit: -> shouldThrowForInvalidSocketSendBufferSize()

mjsax · 2018-09-21T04:26:19Z

+    }
+
+    @Test(expected = KafkaException.class)
+    public void testInvalidSocketReceiveBufferSize() {


nit: -> shouldThrowForInvalidSocketReceiveBufferSize()

…in KafkaStreams (apache#5643) What changes were proposed in this pull request? atLeast(0) in StreamsConfig, ProducerConfig and ConsumerConfig were replaced by SEND_BUFFER_LOWER_BOUND and RECEIVE_BUFFER_LOWER_BOUND from CommonClientConfigs. How was this patch tested? Three unit tests were added to KafkaStreamsTest Reviewers: Guozhang Wang <guozhang@confluent.io>, John Roesler <john@confluent.io>, Matthias J. Sax <mjsax@apache.org>

mjsax added the streams label Sep 12, 2018

aai95 force-pushed the 7379-constraint-in-StreamsConfig branch from 5d5dde0 to a3b0401 Compare September 13, 2018 13:09

aai95 force-pushed the 7379-constraint-in-StreamsConfig branch from a3b0401 to 762b2d2 Compare September 14, 2018 11:16

aai95 force-pushed the 7379-constraint-in-StreamsConfig branch from 762b2d2 to e609518 Compare September 18, 2018 12:49

mjsax reviewed Sep 18, 2018

View reviewed changes

aai95 added 2 commits September 20, 2018 15:08

MINOR: send.buffer.bytes should be allowed to set -1 in KafkaStreams (a…

6a74b79

…pache#7379)

Three unit tests were added to KafkaStreamsTest.java

ead236f

aai95 force-pushed the 7379-constraint-in-StreamsConfig branch from e609518 to ead236f Compare September 20, 2018 12:49

guozhangwang approved these changes Sep 20, 2018

View reviewed changes

guozhangwang merged commit 7bc1019 into apache:trunk Sep 20, 2018

mjsax reviewed Sep 21, 2018

View reviewed changes

Uh oh!

Conversation

aai95 commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Committer Checklist (excluded from commit message)

Uh oh!

aai95 commented Sep 12, 2018

Uh oh!

vvcephei commented Sep 12, 2018

Uh oh!

aai95 commented Sep 13, 2018

Uh oh!

mjsax commented Sep 13, 2018

Uh oh!

aai95 commented Sep 14, 2018

Uh oh!

aai95 commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjsax Sep 18, 2018

Choose a reason for hiding this comment

Uh oh!

guozhangwang Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Sep 19, 2018

Uh oh!

aai95 commented Sep 20, 2018

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Sep 20, 2018

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax Sep 21, 2018

Choose a reason for hiding this comment

Uh oh!

mjsax Sep 21, 2018

Choose a reason for hiding this comment

Uh oh!

mjsax Sep 21, 2018

Choose a reason for hiding this comment

Uh oh!

mjsax Sep 21, 2018

Choose a reason for hiding this comment

Uh oh!

mjsax Sep 21, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aai95 commented Sep 12, 2018 •

edited

Loading

aai95 commented Sep 18, 2018 •

edited

Loading