Skip to content

Conversation

@lordcheng10
Copy link
Contributor

@lordcheng10 lordcheng10 commented Jun 20, 2022

Motivation

Fix the queue size in writeThreadPool exceeds the configured size.

I print out the queue size every time I submit a task, and then set the queue size to 1:
maxPendingAddRequestsPerThread=1

The queue size of the log output is greater than 1:

image

It means that the queue size set by maxPendingAddRequestsPerThread does not limit the queue size.

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

rerun failure checks

5 similar comments
@lordcheng10
Copy link
Contributor Author

rerun failure checks

@lordcheng10
Copy link
Contributor Author

rerun failure checks

@lordcheng10
Copy link
Contributor Author

rerun failure checks

@lordcheng10
Copy link
Contributor Author

rerun failure checks

@lordcheng10
Copy link
Contributor Author

rerun failure checks

@lordcheng10
Copy link
Contributor Author

@hangc0276 @eolivelli PTAL,thanks!

@lordcheng10
Copy link
Contributor Author

@shoothzj PTAL,thanks!

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.

This change will cause an incredible amount of mutex contention and big performance regression.

@lordcheng10
Copy link
Contributor Author

lordcheng10 commented Jun 20, 2022

This change will cause an incredible amount of mutex contention and big performance regression.

Do you have any good idea about the queue size exceeding the configured size? At present, the queue size we configured cannot control the queue size in high concurrency scenarios. @merlimat

@lordcheng10
Copy link
Contributor Author

incredible

@merlimat OK, I will try some other solutions.

public void execute(Runnable command) {
this.checkQueue(1);
super.execute(command);
synchronized (this) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a style note..... you can push synchronized to the method signature

@dlg99
Copy link
Contributor

dlg99 commented Jun 21, 2022

@lordcheng10 +1 to Matteo's concern about performance.
Original implementation #1309 concentrated on "lightweight" over "precise size" hence it is optimistic and I think this is ok.
checkQueue() and methods using it are not synchronized, it is possible to call it from multiple threads, pass the check and end up with more tasks in queue than the soft limit. Performance benefits eher outweigh the precision.

Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

+1 to Matteo's concern about performance.

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.

5 participants