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

New feature - a concurrency limiter #22

Merged
merged 5 commits into from Dec 16, 2015
Merged

Conversation

spkrka
Copy link
Member

@spkrka spkrka commented Dec 12, 2015

No description provided.

@spkrka spkrka force-pushed the krka/concurrency_limiter branch 3 times, most recently from 038722a to 793dd80 Compare December 12, 2015 09:18
return future;
}
}
}
Copy link

Choose a reason for hiding this comment

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

Consider adding missing newline.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@odsod
Copy link

odsod commented Dec 12, 2015

+1

Very nice, this will come in handy.

@pettermahlen
Copy link
Member

Looks good, a couple of questions, though:

  • did you consider (optionally) using a BlockingQueue, so as to enforce size limits? The add() method of a BlockingQueue will throw when trying to add beyond a certain limit.
  • the size() method of ConcurrentLinkedQueue is pretty slow, according to its javadocs, so calling numQueued() may be expensive. Should that be documented somehow?

@spkrka
Copy link
Member Author

spkrka commented Dec 14, 2015

@pettermahlen Thanks for the comments.

I pushed some new commits which leads to:

  • Creating a limiter requires specifying a maximum queue size. Unbounded queues are dangerous after all.
  • Calling add may throw an exception if the maximum queue size is exceeded.
  • Instead of calling queue.size(), the code tracks the size manually with an AtomicInteger.

final SettableFuture<T> response = SettableFuture.create();
final Job<T> job = new Job<T>(callable, response);
if (queueSize.get() >= maxQueueSize) {
throw new CapacityReachedException("Queue size has reached capacity: " + maxQueueSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like mixing sync exc with async exc.

@rouzwawi
Copy link
Member

I'm a bit unsure if the calls to pump() from the future callback in invoke() is such a good idea. It will cause the callables in the queue to be invoked from arbitrary other thread pools in the application using this utility.

It might be OK from a cpu utilization view since the callables should return immediatly, but can cause confusion for other surprising reasons.

@spkrka
Copy link
Member Author

spkrka commented Dec 16, 2015

Yes, callables will indeed run on seemingly random threads. I don't think this is enough of a problem to block releasing the feature. If it turns out to be a real problem we can create a new API that lets people pass in an executor to run on.

spkrka added a commit that referenced this pull request Dec 16, 2015
New feature - a concurrency limiter
@spkrka spkrka merged commit 588a3c5 into master Dec 16, 2015
@spkrka spkrka deleted the krka/concurrency_limiter branch December 16, 2015 09:40
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

5 participants