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

HysterixCommandProperties.Setter().withExecutionIsolationThreadTimeoutInMilliseconds deprecated with no reason given and still used in the documentation #740

Closed
darklajid opened this issue Apr 4, 2015 · 9 comments

Comments

@darklajid
Copy link

See the title.

I got some code that uses Hystrix, containing

super(Setter.withGroupKey(HystrixCommandGroupKey.Factory.asKey("SomeKey")) .andCommandKey(HystrixCommandKey.Factory.asKey("AnotherKey" + Integer.toString(myTimeout))) .andCommandPropertiesDefaults(HystrixCommandProperties.Setter() .withExecutionIsolationThreadTimeoutInMilliseconds(myTimeout)));

and I get compilation warnings with 1.4.3 about the last line calling a deprecated withExecutionIsolationThreadTimeoutInMilliseconds.

Sure enough, the javadocs have a deprecated tag. But no reason given and I wouldn't know if that code should be migrated to something else (what?).

It doesn't help that docs in the wiki still reference that method at least in one place:
https://github.com/Netflix/Hystrix/wiki/Configuration#execution.isolation.strategy

Should the configuration example be changed? Could you add a @deprecated JavaDoc comment to point people like me to a reason/migration path?

@mattrjacobs
Copy link
Contributor

Thanks for the report, @darklajid . The deprecation was intentional. I'll get the documentation cleaned up.

@mattrjacobs
Copy link
Contributor

Just removed all references to this method in the Wiki

@mattrjacobs
Copy link
Contributor

Just uploaded Javadoc in #744.

@darklajid
Copy link
Author

Hey. Thanks for the quick turnaround. Not trying to hijack this bug, but it's the same javadoc subject.

HysterixCommandProperties.Setter has this example in the javadocs, for the class (i.e. right at the top of https://netflix.github.io/Hystrix/javadoc/com/netflix/hystrix/HystrixCommandProperties.Setter.html

HystrixCommandProperties.Setter() .setExecutionTimeoutInMilliseconds(100) .setExecuteCommandOnSeparateThread(true);

Can't find those. Could that need a s/setE/withE/ perhaps?

@mattrjacobs
Copy link
Contributor

Yes, you're right. Will get that fixed and re-uploaded. Thanks for the
catch!

On Tue, Apr 7, 2015 at 6:37 AM, Benjamin Podszun notifications@github.com
wrote:

Hey. Thanks for the quick turnaround. Not trying to hijack this bug, but
it's the same javadoc subject.

HysterixCommandProperties.Setter has this example in the javadocs, for the
class (i.e. right at the top of
https://netflix.github.io/Hystrix/javadoc/com/netflix/hystrix/HystrixCommandProperties.Setter.html

HystrixCommandProperties.Setter()
.setExecutionTimeoutInMilliseconds(100)
.setExecuteCommandOnSeparateThread(true);

Can't find those. Could that need a s/setE/withE/ perhaps?


Reply to this email directly or view it on GitHub
#740 (comment).

@mattrjacobs
Copy link
Contributor

I just reuploaded in #747. Can you take a look and see if you find any further issues? I'd appreciate that.

@darklajid
Copy link
Author

I .. guess that's close. :)
Seriously though, seems good for this class. I did find the same problem (s/set/with/) elsewhere [1]. Sorry for the nitpicking.

1: https://netflix.github.io/Hystrix/javadoc/com/netflix/hystrix/HystrixThreadPoolProperties.Setter.html

@mattrjacobs
Copy link
Contributor

I just reuploaded in #756 to fix the HystrixThreadPoolProperties.Setter s/with/set issue.

If you spot more Javadoc inconsistencies, please keep pointing them out. It absolutely helps the project.

Is it OK to close this issue now, and any other Javadoc issues can get opened as a new issue?

@mattrjacobs
Copy link
Contributor

Closing. If there are further Javadoc issues, please open a new issue

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