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

Proposed fix for issue #414 #428

Merged
merged 2 commits into from Sep 8, 2020

Conversation

orpiske
Copy link
Contributor

@orpiske orpiske commented Sep 7, 2020

No description provided.

Copy link
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

LGTM, lets wait for @valdar

@orpiske
Copy link
Contributor Author

orpiske commented Sep 7, 2020

LGTM, lets wait for @valdar

Thanks.

For context and future reference, here's some numbers from a test I ran on Friday: #414 (comment)

@oscerd
Copy link
Contributor

oscerd commented Sep 7, 2020

@orpiske can you please rebase so your branch will pick up the newly poiting to master gh actions? Thanks

@orpiske
Copy link
Contributor Author

orpiske commented Sep 7, 2020

@orpiske can you please rebase so your branch will pick up the newly poiting to master gh actions? Thanks

Sure thing.

This adds a reference implementation for checking the resource usage of
the RabbitMQ component while iddle. The motivation for this is related
to the github issue apache#414.
@orpiske orpiske force-pushed the perf-test-rabbitmq-alternative branch from b4143e6 to 4b7b0f6 Compare September 7, 2020 15:27
@orpiske
Copy link
Contributor Author

orpiske commented Sep 7, 2020

I think it should be ok now, @oscerd

@oscerd
Copy link
Contributor

oscerd commented Sep 7, 2020

Thanks, I think @valdar is reviewing.

@valdar
Copy link
Member

valdar commented Sep 7, 2020

Hi @orpiske great work!

I was wondering which one of the contributions is predominant: better handling the remaining time or returning null if there are no results.

Do you mind, since you already have the performance tests at hand, doing a quick comparison towards just returning null if the result set is empty, like: valdar@2edcfc0

@orpiske
Copy link
Contributor Author

orpiske commented Sep 8, 2020

Hi @orpiske great work!

Thanks! And thanks for the review!

I was wondering which one of the contributions is predominant: better handling the remaining time or returning null if there are no results.

The biggest gain is related to avoiding the busy spin. On the previous code we were returning immediately from the receiveNoWait and so the loop was running thousands of times per poll.

The change replaces the busy spin with a direct call to the receive, blocking for at most maxPollDuration.

Do you mind, since you already have the performance tests at hand, doing a quick comparison towards just returning null if the result set is empty, like: valdar@2edcfc0

I tried with returning the null as well. I have a flame graph with that test with null available here.

It doesn't seem to affect the performance, but I changed it nonetheless because that way it complies with the connect API.

This modifies the code so that it blocks while waiting for the messages
to arrive while also respecting the maxPollDuration.
@orpiske orpiske force-pushed the perf-test-rabbitmq-alternative branch from 4b7b0f6 to 4bcba9f Compare September 8, 2020 05:12
@valdar
Copy link
Member

valdar commented Sep 8, 2020

@orpiske thanks for the insight on this, I thought it was something similar but I just wanted to be sure.

@oscerd
Copy link
Contributor

oscerd commented Sep 8, 2020

I believe after merging this we could cut a 0.5.0 release

@orpiske
Copy link
Contributor Author

orpiske commented Sep 8, 2020

I believe after merging this we could cut a 0.5.0 release

+1 from me. I think we could start preparing that.

@orpiske
Copy link
Contributor Author

orpiske commented Sep 8, 2020

Looks like everything is alright: approvals OK, CI ok. So, merging this one.

@orpiske orpiske merged commit 0f26f98 into apache:master Sep 8, 2020
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.

None yet

3 participants