Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed message rate out with batches to count messages/s (#466) #474

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

jai1
Copy link
Contributor

@jai1 jai1 commented Jun 14, 2017

Fix for #466

Don't Merge yet

@jai1 jai1 added the type/bug The PR fixed a bug or issue reported a bug label Jun 14, 2017
@jai1 jai1 added this to the 1.18 milestone Jun 14, 2017
@jai1 jai1 self-assigned this Jun 14, 2017
@jai1 jai1 requested review from merlimat and rdhabalia June 14, 2017 18:32
}
});
t1.start();
Thread.sleep(60000); // one minute wait for stats to be updated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't add one more minute to unit tests :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced the sleep to two second and manually call the updateRates function.
Reducing the sleep to one second causes the rate to be shown as 0.

@@ -185,8 +186,10 @@ public String consumerName() {
readChecksum(metadataAndPayload);
}

MessageMetadata messageMetaData = Commands.parseMessageMetadata(metadataAndPayload);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't have to deserialize messageMetadata again. at line #160 we already know number of messages which is going to be dispatched. can we use that number?

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

pulsar.getBrokerService().updateRates();
double actualRate = admin.persistentTopics().getStats(topicName).msgRateOut;
log.info("actualRate : {}, produceRate : {}", actualRate, produceRate);
assertTrue((produceRate - 1) <= actualRate && actualRate <= (produceRate + 1));
Copy link
Contributor

@merlimat merlimat Jun 14, 2017

Choose a reason for hiding this comment

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

@jai1 Can you run this test multiple times to ensure is not intermittently failing?

@merlimat
Copy link
Contributor

@jai1 The new test is failing

Failed tests: 
  SimpleProducerConsumerStatTest.testBatchMessagesRateOut:359 expected [true] but found [false]

@jai1 jai1 merged commit 042521a into apache:master Jun 15, 2017
@jai1 jai1 deleted the rateOutTest branch June 15, 2017 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants