Skip to content

KAFKA-10768 Make ByteBufferInputStream.read(byte[], int, int) to follow the contract#9761

Merged
chia7712 merged 6 commits intoapache:trunkfrom
bertber:MINOR-VerifyByteBufferInputStream
Jan 3, 2021
Merged

KAFKA-10768 Make ByteBufferInputStream.read(byte[], int, int) to follow the contract#9761
chia7712 merged 6 commits intoapache:trunkfrom
bertber:MINOR-VerifyByteBufferInputStream

Conversation

@bertber
Copy link
Copy Markdown
Contributor

@bertber bertber commented Dec 16, 2020

I made a test for ByteBufferInputStream in the ByteBufferLogInputStreamTest.
First, I add a ByteBuffer that it's not empty to the ByteBufferInputStream, in order to verify it.
After that, I try to use ByteBufferInputStream's read function and check return value whether it's correct.

Committer Checklist (excluded from commit message)

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

…tStreamTest

I made a test for ByteBufferInputStream in the ByteBufferLogInputStreamTest.
First, I add a ByteBuffer that it's not empty to the ByteBufferInputStream, in order to verify it.
After that, I try to use ByteBufferInputStream's read function and check return value whether it's correct.
@chia7712
Copy link
Copy Markdown
Member

@bertber Please take a look at origin PR (https://github.com/apache/kafka/pull/3752/files#diff-ae627decd5dd27d053a7a9b051860f60df6b89c8884e50df2f9ae0de6a4645e5R40). It would be better to fix the scenario of 'len==0'.

}

@Test
public void testReadUnsignedIntFromInputStream() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you move the tests to ByteBufferInputStreamTest? If there is no such test class, feel free to create new one.

@chia7712
Copy link
Copy Markdown
Member

Is kafka.server.ControllerMutationQuotaTest.testPermissiveDeleteTopicsRequest related to this PR?

@bertber
Copy link
Copy Markdown
Contributor Author

bertber commented Dec 18, 2020

Is kafka.server.ControllerMutationQuotaTest.testPermissiveDeleteTopicsRequest related to this PR?

No, I don't think it is related to this PR.

testReadUnsignedIntFromInputStream from ByteBufferLogInputStreamTest to
ByteufferInputStreamTest.
assertNotNull(logInputStream.nextBatch());
logInputStream.nextBatch();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please revert this change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This change have been reverted.

public class ByteBufferInputStreamTest {

@Test
public void testReadUnsignedIntFromInputStream() throws Exception {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

throws Exception is useless.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It have been removed.

*/
package org.apache.kafka.common.utils;

import static org.junit.Assert.assertEquals;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have a pending PR offering a consistent import order (see #8404 (comment)). Could you please follow the rules (although it is not merged)?

  1. kafka, org.apache.kafka
  2. com, net, org
  3. java, javax

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have put the java behind the org

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.

@bertber This patch LGTM. one small comment is left. Please take a look

@chia7712 chia7712 changed the title KAFKA-10768 Add a test for ByteBufferInputStream to ByteBufferLogInputStreamTest KAFKA-10768 Make ByteBufferInputStream.read(byte[], int, int) to follow the contract Dec 31, 2020
@chia7712 chia7712 merged commit bdb29e4 into apache:trunk Jan 3, 2021
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