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

Implementing PriorityScheduler based on ExecutorScheduler #4

Open
NorseDreki opened this issue Mar 9, 2015 · 4 comments
Open

Implementing PriorityScheduler based on ExecutorScheduler #4

NorseDreki opened this issue Mar 9, 2015 · 4 comments

Comments

@NorseDreki
Copy link

@ronshapiro, what do you think about implementation of PriorityScheduler which is based on standard ExecutorScheduler?

There might be a PriorityBlockingQueue accepting PriorityExecutorActions as a repacement for ExecutorScheduler's ConcurrentLinkedQueue with ExecutorAction.

@ronshapiro
Copy link
Owner

Do you have anything in mind? What would be the added benefit?

FTR, here's a discussion in Guava about a possible implementation of a PriorityExecutorService - google/guava#888 but I'm not sure if it will be implemented.

@NorseDreki
Copy link
Author

For me the motivation is to keep it simple, go the shortest path and use what RxJava offers out of the box (ExecutorScheduler). I cannot name many benefits except that since it's standard -- it's tested; so does it also follows "the Worker contract" as mentioned in the discussion for this commit: 4f49449.

Here is a sketch how this could look like: upelsin@c50448cf91feca84cdcf1c0945126f6240436b21.

@ronshapiro
Copy link
Owner

I found there to be actually more complexity in this implementation and would still need tests since it only marginally uses the ExecutorSchedulerWorker. PriorityScheduler.withConcurrency(1) solves the comment in that commit about concurrency, but part of what makes this nice is giving you the flexibility to create multiple threads if you like (even if it's not the intended use). I do like your idea of using a ScheduledExecutorService for the executor so that it can handle schedule(Action0, long, TimeUnit) appropriately, I think that should be included.

@NorseDreki
Copy link
Author

Your comments make sense, thanks for the discussion.

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

No branches or pull requests

2 participants