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

Mismatch in isExecutedInThread documentation #448

Closed
mattrjacobs opened this issue Jan 7, 2015 · 3 comments
Closed

Mismatch in isExecutedInThread documentation #448

mattrjacobs opened this issue Jan 7, 2015 · 3 comments
Milestone
1.4.0-RC6

Comments

@mattrjacobs
Copy link
Contributor

In AbstractCommand.isExecutedInThread(), the Javadoc states:

This specifies if a thread execution actually occurred, not just if it is configured to be executed in a thread.

Tests in HystrixObservableCommandTest assert something slightly different.

For the tests testRejectionWithFallbackRequestContextWithThreadIsolatedSynchronousObservable, testRejectionWithFallbackRequestContextWithThreadIsolatedAsynchronousObservable, testRejectionWithFallbackRequestContextWithThreadIsolatedAsynchronousObservableAndCapturedContextScheduler, they all have this assertion:

// thread isolated so even though we're rejected we mark that it attempted execution in a thread

These are at odds with one another.

/cc @benjchristensen for insight into what might break if this behavior changes to match the Javadoc in AbstractCommand.isExecutedInThread()
@mattrjacobs mattrjacobs added this to the 1.4.0-RC6 milestone Jan 7, 2015
@mattrjacobs
Copy link
Contributor Author

While working on a fix for #377, I came across this mismatch.

@mattrjacobs
Copy link
Contributor Author

My inclination is to change the behavior to match the Javadoc (only return true if the run() method was invoked on the Hystrix thread), since you can already query the HystrixCommandProperties on whether or not the command is configured to use SEMAPHORE or THREAD isolation.

mattrjacobs pushed a commit to mattrjacobs/Hystrix that referenced this issue Jan 7, 2015
Move the marking of the thread pool completion to the end of the Obse…
b0808cb 
…rvable chain (Netflix#377)

* Both thread pool metrics and the onThreadComplete execution hook now run later
* Modified behavior of HystrixCommand.isExecutedInThread() to match the Javadoc (Netflix#448)
** Now this returns true iff the Hystrix thread executed the run() method
@mattrjacobs mattrjacobs mentioned this issue Jan 7, 2015
Merged
@mattrjacobs
Copy link
Contributor Author

Closed via #449

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4.0-RC6
Development

No branches or pull requests
1 participant
@mattrjacobs