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

Look at simplifying future handling for SubmitterSchedulerLimiter and PrioritySchedulerLimiter #50

Closed
jentfoo opened this issue Dec 31, 2013 · 2 comments
Closed

Look at simplifying future handling for SubmitterSchedulerLimiter and PrioritySchedulerLimiter #50

jentfoo opened this issue Dec 31, 2013 · 2 comments
Assignees
@jentfoo
Labels
enhancement
Milestone
1.0.0 Release!

Comments

@jentfoo
Copy link
Member

jentfoo commented Dec 31, 2013

We should consider if we want to or can simplify the Future handling for SubmitterSchedulerLimiter and PrioritySchedulerLimiter. Right now we have this concept of a "FutureListenableFuture" which provides a future which depends on the implementation of another Future coming in later.

In the ExecutorLimiter however we are just wrapping the task in a ListernableFutureTask and calling to execute. I think we could simplify things by doing this same behavior for the other scheduler limiters. This would also allow us to take in a SimpleSchedulerInterface as the scheduler in the constructor and fulfill the SubmitterSchedulerInterface.
@ghost ghost assigned jentfoo Dec 31, 2013
jentfoo pushed a commit that referenced this issue Dec 31, 2013
A little progress towards Issue #50. This combines SimpleSchedulerLim…
4783b2b 
…iter and SubmitterSchedulerLimiter into one (simpler) class called "SchedulerLimiter".

This reduced a lot of code, as well as provided a MUCH simpler simplementation compared to what SubmitterSchedulerLimiter used to be.  This change also exposed a defect in PriorityScheduledExecutor with removal of callables which are wrapped in runnables.  This commit fixes that in order to fix the unit tests.

We still need to look at what we are going to do for the PrioritySchedulerLimiter, but I may wait on that for now, since those changes will be more dramatic (and possibly more risky).  It was important for me to do these before 1.0.0 release because these changes break the API/interface.
@jentfoo
Copy link
Member Author

jentfoo commented Dec 31, 2013

At a minimum for 1.0.0 I made this change with respect to the SimpleSchedulerLimiter and the SubmitterSchedulerLimiter (which were combined into just a 'SchedulerLimiter').

It was important to do that now because that changes the API (so it otherwise would have had to wait to 2.0.0).

I may or may not do the PrioritySchedulerLimiter before 1.0.0. If I don't, it likely will be included in a 1.1.0 release. I will need to think about it and decide later.

jentfoo pushed a commit that referenced this issue Dec 31, 2013
Because the PrioritySchedulerLimiter was the only user of the Abstrac…
357f634 
…tSchedulerLimiter. I decided to bring the code within that class back into the PrioritySchedulerLimiter.

There are no functional changes, but since Issue #50 wants to change how we are doing the PrioritySchedulerLimiter, it made sense to bring the code in question closer together.
@jentfoo
Copy link
Member Author

jentfoo commented Dec 31, 2013

Because the text of this is no longer relevant after the most recent changes, I am going to close this issue, and open a new one for the PrioritySchedulerLimiter

@jentfoo jentfoo closed this as completed Dec 31, 2013
This was referenced Dec 31, 2013
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jentfoo jentfoo

Labels
enhancement
Milestone
1.0.0 Release!
Development

No branches or pull requests
1 participant
@jentfoo