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

Context scheduler #28

Merged
merged 6 commits into from Jan 15, 2016
Merged

Context scheduler #28

merged 6 commits into from Jan 15, 2016

Conversation

vietj
Copy link
Contributor

@vietj vietj commented Jan 8, 2016

Community patch by @kevlarC + improvements.

@kevlarC would you mind to review the pull request please ?

KevinJCross and others added 4 commits January 4, 2016 15:35
If you call an observable that calls back on a different thread the offload via observeOn (Scheduler) does not return on the correct thread.
In our case we were calling couchbase rxApi for inserting ext and it returns on an internal thread. Offloading it makes scense so we do not pollute our vertx thread with couchbase threads. In this case we get the wrong thread back and it causes some erratic bahaviour later on.

 @see SchedulerTest.java:testScheduleObserveOnReturnsOnCorrectThread
 @see ContextScheduler.java
@KevinJCross
Copy link
Contributor

@vietj I do have some suggestions to reduce the blocking of the ContextScheduler class. Might need some performance style tests to see if it actually is worth putting in though.
Should I just create another pull request to show the changes.

@KevinJCross
Copy link
Contributor

@vietj Im not sure about the blocking side that you requested changes for though ?

@vietj
Copy link
Contributor Author

vietj commented Jan 13, 2016

@kevlarC don't mind about the blocking side, it was an odd suggestion

@vietj
Copy link
Contributor Author

vietj commented Jan 13, 2016

@kevlarC yes provide another PR

@vietj
Copy link
Contributor Author

vietj commented Jan 13, 2016

@kevlarC what do you mean by X thread ? the test uses new Thread() { ... } it's not a Vert.x thread

@vietj
Copy link
Contributor Author

vietj commented Jan 13, 2016

btw do you know you can comment inline on the commits, it would be easier rather than pointing the line :-) ?

@KevinJCross
Copy link
Contributor

It took a little bit more review and I like the tests they are a bit simpler. Although a little less 'life like' 😉 .
I've only been doing pull requests for a short time and just shortly after I submitted this did I find out about the inline comments 😄

Otherwise it looks good to me

@vietj vietj merged commit 0e51de8 into master Jan 15, 2016
@vietj vietj removed the to review label Jan 15, 2016
@vietj vietj deleted the context-scheduler branch January 15, 2016 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants