Skip to content

handle MP_Fault_Tolerance_NonFallback_Enabled properly #121

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
Merged
merged 1 commit into from
Oct 4, 2019
Merged

handle MP_Fault_Tolerance_NonFallback_Enabled properly #121

merged 1 commit into from
Oct 4, 2019

Conversation

kenfinnigan
Copy link
Contributor

@kenfinnigan kenfinnigan commented Oct 3, 2019 •
edited by Ladicek
Loading

Fixes #120

@kenfinnigan @Ladicek
handle MP_Fault_Tolerance_NonFallback_Enabled properly
f82220a 
Before this commit, setting `MP_Fault_Tolerance_NonFallback_Enabled`
to `false` globally disabled the fault tolerance strategies (except
`@Fallback`) without the ability to selectively enable individual
classes/methods via configuration. Such behavior is against the spec;
selectively enabling individual classes/methods shall be possible.
Unfortunately, there's also bug in the TCK, so that this behavior
isn't tested properly. The TCK bug is being fixed in MP FT itself.
Ladicek
Ladicek approved these changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've replaced annotationType.getSimpleName().equals("Fallback") by annotationType.equals(Fallback.class) and improved the commit message, but otherwise LGTM.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 4, 2019

I'm just thinking if you perhaps compared strings instead of classes intentionally, because comparing classes failed in Quarkus? If that's the case, and also considering microprofile/microprofile-fault-tolerance#450, I'd say there are some serious classloading bugs in Quarkus itself, as this simply must work.

@Ladicek Ladicek added this to the 2.0.13 milestone Oct 4, 2019
@Ladicek Ladicek changed the title Fixes #120 handle MP_Fault_Tolerance_NonFallback_Enabled properly Oct 4, 2019
@Ladicek Ladicek merged commit 61b0365 into smallrye:master Oct 4, 2019
@kenfinnigan kenfinnigan deleted the fix-120 branch October 4, 2019 13:12
@kenfinnigan
Copy link
Contributor Author

I've verified that the change still works on Quarkus, not sure why I didn't use the class directly.

In the future, it would be appreciated if you could ask about making changes to the PR before doing them yourself. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladicek Ladicek Ladicek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.

Bug with MP_Fault_Tolerance_NonFallback_Enabled
2 participants
@kenfinnigan @Ladicek