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.

Sign up for GitHub

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

Jump to bottom

Set Block If queue full to false by default #441

Merged
merged 3 commits into from
Jun 2, 2017
Merged

Set Block If queue full to false by default #441

merged 3 commits into from
Jun 2, 2017

Conversation

jai1
Copy link
Contributor

@jai1 jai1 commented May 31, 2017

Motivation

Fix for #422

Modifications

  • Changed default options in CPP and Java Client ProducerConfigurations
  • Fixed some test cases
  • Changed perf tests to block if queue is full

Result

sendAsync becomes a non-blocking call

Do not merge this

@jai1 jai1 added c++ area/client type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels May 31, 2017
@jai1 jai1 added this to the 1.18 milestone May 31, 2017
@jai1 jai1 self-assigned this May 31, 2017
@jai1 jai1 requested review from merlimat, saandrews and rdhabalia May 31, 2017 18:09
@msb-at-yahoo
Copy link
Contributor

how is the default queue size change related?

@jai1
Copy link
Contributor Author

jai1 commented May 31, 2017 •
edited
Loading

In production, if we have a producer with the following config:-
a. Queue size 1K
b. Produce rate = 2K msg/s

And lets say the broker has a GC pause of 500 milliseconds or a rolling restart is going on then:-
a. With current code base the sendAsync will block for few milliseconds and client will not even know that there was some issue (except high latency)
b. After this PR the client will start seeing failures and will come to the conclusion that the service is down or not working.

So in order to prevent clients seeing failures, we assumed that most properties will not produce over 1K MPS and so a queue size of 30K will prevent them from seeing failures before 30 seconds (default timeout).

But yes I expect there to be some discussion regarding the queue size before the PR is merged

@merlimat @saandrews @rdhabalia - do you agree with this approach?

@msb-at-yahoo
Copy link
Contributor

i think it'd be better to separate the queue size change from the block if full change. both might make sense, but they're not related.

merlimat
merlimat reviewed Jun 1, 2017
View reviewed changes
pulsar-client/src/main/java/com/yahoo/pulsar/client/api/ProducerConfiguration.java Outdated
private int maxPendingMessages = 1000;
private boolean blockIfQueueFull = true;
private int maxPendingMessages = 30000;
private boolean blockIfQueueFull = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also update the Javadoc on the getter/setter

@jai1
Copy link
Contributor Author

jai1 commented Jun 1, 2017

@merlimat - Updated the docs
@msb-at-yahoo - Moving the maxPendingMessages changes to separate PR #445

saandrews
saandrews reviewed Jun 1, 2017
View reviewed changes
Copy link
Contributor

@saandrews saandrews left a comment

Choose a reason for hiding this comment

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

Should we also call it out this change in the documentation?

@jai1
Copy link
Contributor Author

jai1 commented Jun 1, 2017

@saandrews - Have made relevant change to Open Source documentation, will update Yahoo wrapper documentation and release notes once the PR is merged.

merlimat
merlimat approved these changes Jun 2, 2017
View reviewed changes
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.

👍

@jai1 jai1 force-pushed the blockingQueue branch from c763b3c to 6dbc98a Compare June 2, 2017 21:36
@jai1 jai1 merged commit 5c6c9c3 into apache:master Jun 2, 2017
@jai1 jai1 deleted the blockingQueue branch June 2, 2017 23:00
@jai1 jai1 mentioned this pull request Jun 9, 2017
Merged
hangc0276 pushed a commit to hangc0276/pulsar that referenced this pull request May 26, 2021
@BewareMyPower
Add request related metrics (apache#441)
c4d1892 
This PR introduces 3 metrics that are related to requests.
- REQUEST_QUEUE_SIZE: the size of `requestQueue` in `KafkaCommandDecoder`.
- REQUEST_QUEUED_LATENCY: the latency between a request enqueues to `requestQueue` and the request is dequeued.
- REQUEST_PARSE: the time to parse a raw request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saandrews saandrews saandrews approved these changes

@merlimat merlimat merlimat approved these changes

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

Assignees

@jai1 jai1

Labels
area/client type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
1.18
Development

Successfully merging this pull request may close these issues.
4 participants
@jai1 @msb-at-yahoo @merlimat @saandrews