Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

The minBytes and maxWait parameters in the fetch request are invalid #702

Merged

Conversation

lordcheng10
Copy link
Contributor

fix : #699

Copy link
Collaborator

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Please add unit tests to verify this change. And you can run mvn checkstyle:check and mvn spotbugs:check to do the basic checks in your local env for code quality.

@lordcheng10
Copy link
Contributor Author

Please add unit tests to verify this change. And you can run mvn checkstyle:check and mvn spotbugs:check to do the basic checks in your local env for code quality.

@BewareMyPower I added a unit test: io.streamnative.pulsar.handlers.kop.KafkaApisTest#testFetchMinBytes

@BewareMyPower
Copy link
Collaborator

I left some comments, PTAL. And I found the test failed, here's my debugging info in a single test:

  • endTime1 - startTime1 = 10029
  • endTime2 - startTime2 = 10006

It looks like even if the fetched bytes are enough, handleFetch still took about 10 seconds.

@lordcheng10
Copy link
Contributor Author

lordcheng10 commented Sep 3, 2021

I left some comments, PTAL. And I found the test failed, here's my debugging info in a single test:

  • endTime1 - startTime1 = 10029
  • endTime2 - startTime2 = 10006

It looks like even if the fetched bytes are enough, handleFetch still took about 10 seconds.

Are there any breakpoints during debugging? I ran the unit test locally, and this case did not appear. @BewareMyPower

@BewareMyPower
Copy link
Collaborator

You can also see the CI result

Error:  Failures: 
Error:    KafkaApisTest.testFetchMinBytes:374 expected [true] but found [false]

@lordcheng10
Copy link
Contributor Author

You can also see the CI result

Error:  Failures: 
Error:    KafkaApisTest.testFetchMinBytes:374 expected [true] but found [false]

In the ci log, I saw a fetch error log. It seemed that the address was not set, which caused the fetch to fail. Therefore, the data volume of the fetch was less than minBytes, resulting in a delay response, but the local environment operation did not report this problem, so I It is suspected to be related to the operating environment:
17:50:39.765 [TestNG-method=testFetchMinBytes-1:io.streamnative.pulsar.handlers.kop.KafkaTopicManager@121] ERROR io.streamnative.pulsar.handlers.kop.KafkaTopicManager-[Mock for Channel, hashCode: 870184972] Try to getTopicConsumerManager(persistent://public/default/testMinBytesTopic-partition-0) while remoteAddress is not set

@lordcheng10
Copy link
Contributor Author

You can also see the CI result

Error:  Failures: 
Error:    KafkaApisTest.testFetchMinBytes:374 expected [true] but found [false]

In the ci log, I saw a fetch error log. It seemed that the address was not set, which caused the fetch to fail. Therefore, the data volume of the fetch was less than minBytes, resulting in a delay response, but the local environment operation did not report this problem, so I It is suspected to be related to the operating environment:
17:50:39.765 [TestNG-method=testFetchMinBytes-1:io.streamnative.pulsar.handlers.kop.KafkaTopicManager@121] ERROR io.streamnative.pulsar.handlers.kop.KafkaTopicManager-[Mock for Channel, hashCode: 870184972] Try to getTopicConsumerManager(persistent://public/default/testMinBytesTopic-partition-0) while remoteAddress is not set

@BewareMyPower

@lordcheng10
Copy link
Contributor Author

The local test is passed:
image

@BewareMyPower
Copy link
Collaborator

We should not rely on the local test result.

You can download the Artifacts from the Details page. We can see the logs under artifacts/tests/target/surefire-reports directory:

$ grep "cost" io.streamnative.pulsar.handlers.kop.KafkaApisTest-output.txt 
04:21:45.299 [TestNG-method=testFetchMinBytes-1:io.streamnative.pulsar.handlers.kop.KafkaApisTest@346] INFO  io.streamnative.pulsar.handlers.kop.KafkaApisTest - cost time1:3011
04:21:48.384 [TestNG-method=testFetchMinBytes-1:io.streamnative.pulsar.handlers.kop.KafkaApisTest@366] INFO  io.streamnative.pulsar.handlers.kop.KafkaApisTest - cost time2:3000

And here's my local test:

image

@lordcheng10
Copy link
Contributor Author

We should not rely on the local test result.

You can download the Artifacts from the Details page. We can see the logs under artifacts/tests/target/surefire-reports directory:

$ grep "cost" io.streamnative.pulsar.handlers.kop.KafkaApisTest-output.txt 
04:21:45.299 [TestNG-method=testFetchMinBytes-1:io.streamnative.pulsar.handlers.kop.KafkaApisTest@346] INFO  io.streamnative.pulsar.handlers.kop.KafkaApisTest - cost time1:3011
04:21:48.384 [TestNG-method=testFetchMinBytes-1:io.streamnative.pulsar.handlers.kop.KafkaApisTest@366] INFO  io.streamnative.pulsar.handlers.kop.KafkaApisTest - cost time2:3000

And here's my local test:

image

I use the kafkaconsumer API instead of calling handlefetchrequest directly to try to solve the problem! @BewareMyPower

Copy link
Contributor

@aloyszhang aloyszhang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

LGTM. Good job!

@BewareMyPower BewareMyPower merged commit 75205e7 into streamnative:master Sep 7, 2021
@BewareMyPower BewareMyPower added the type/feature Indicates new functionality label Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/feature Indicates new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] The minBytes and maxWait parameters in the fetch request are invalid
3 participants