Skip to content

KAFKA-6891: send.buffer.bytes should be allowed to set -1 in KafkaConnect#13398

Merged
chia7712 merged 5 commits intoapache:trunkfrom
garyparrot:fix-connect-sock-config-typo
Mar 21, 2023
Merged

KAFKA-6891: send.buffer.bytes should be allowed to set -1 in KafkaConnect#13398
chia7712 merged 5 commits intoapache:trunkfrom
garyparrot:fix-connect-sock-config-typo

Conversation

@garyparrot
Copy link
Copy Markdown
Contributor

@garyparrot garyparrot commented Mar 16, 2023

What changes were proposed in this pull request?

Fix the incorrect valid range of the following Kafka connect configs: send.buffer.bytes and receive.buffer.bytes.

image

Currently, the valid values start from 0, although the documentation states -1 is also a valid value.

How was this patch tested?

Add relevant unit test to DistributedConfigTest.

Committer Checklist (excluded from commit message)

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

@chia7712
Copy link
Copy Markdown
Member

@garyparrot nice catch. It is not only typo but also incorrect validation. the atLeast(0) disables users to input -1 for socket buffer. Hence, this PR is fixing a bug rather than a typo. If this is related to a bug, could you file a JIRA with new topic and description. That can help reader in the future to trace similar issue. thanks

@garyparrot garyparrot changed the title MINOR: Fix typos in valid range of socket buffer size KAFKA-6891: send.buffer.bytes should be allowed to set -1 in KafkaConnect Mar 17, 2023
@garyparrot
Copy link
Copy Markdown
Contributor Author

@chia7712 Thanks for your opinion. Indeed this issue should be treated as a bug (The contribution guide has addressed this, sorry I am not familiar with the rules here).

I find this issue has already been mentioned at KAFKA-6891. I have self-assigned that issue and updated the description.

Please take another look, thanks.

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. thanks for this contribution.

@chia7712
Copy link
Copy Markdown
Member

@garyparrot could you rebase PR to trigger QA again?

@chia7712
Copy link
Copy Markdown
Member

linked to stale PR (#4995)

@garyparrot
Copy link
Copy Markdown
Contributor Author

Sure thing.

@chia7712
Copy link
Copy Markdown
Member

@garyparrot could you merge trunk to trigger QA again?

@chia7712
Copy link
Copy Markdown
Member

[2023-03-20T05:31:14.475Z] BUILD SUCCESSFUL in 1h 44m 21s
[2023-03-20T05:31:14.475Z] 224 actionable tasks: 120 executed, 104 up-to-date

the tests pass. will merge it later

@chia7712 chia7712 merged commit ccda370 into apache:trunk Mar 21, 2023
@chia7712
Copy link
Copy Markdown
Member

@garyparrot thanks for this contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants