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

Add retry feature to async feign #1757

Merged
merged 11 commits into from Oct 9, 2022

Conversation

wplong11
Copy link
Collaborator

@wplong11 wplong11 commented Sep 18, 2022

Resolves #1758
Contains #1756 PR commits

@wplong11 wplong11 marked this pull request as draft September 18, 2022 07:32
@wplong11 wplong11 force-pushed the 4-add-retry-feature-to-async-feign branch 4 times, most recently from 9e1ac8c to 46c098b Compare September 19, 2022 23:56
@wplong11 wplong11 force-pushed the 4-add-retry-feature-to-async-feign branch from c6966b0 to 0633e1e Compare September 20, 2022 05:40
@wplong11 wplong11 force-pushed the 4-add-retry-feature-to-async-feign branch from 0633e1e to 1379437 Compare September 20, 2022 05:54
@wplong11 wplong11 marked this pull request as ready for review September 20, 2022 06:05
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

@kdavisk6 do you want to take a look too?

I think this will break binary compatibility, so, major release?

@wplong11
Copy link
Collaborator Author

@velo What classes are you concerned about breaking binary compatibility?(I know a little about Java binary compatibility) If you're concerned about breaking binary compatibility for the Async feature, isn't it ok because it's experimental?

@velo
Copy link
Member

velo commented Sep 28, 2022

@velo , isn't it ok because it's experimental?

Yeah, it's ok. But, there will be complaints:/

@wplong11
Copy link
Collaborator Author

wplong11 commented Sep 28, 2022

@velo What changes break AsyncFeign's Binary Compatability? If the problem is the change from abstract to final, how about removing the final keyword? After that, how about adding the final keyword as a separate PR?

@wplong11
Copy link
Collaborator Author

wplong11 commented Sep 28, 2022

Are you already planning to release the coroutine Feign to version 12? 45fe10a
If so, I think it would be okay if this PR was also released as Feign 12 and I just wonder when that time will be.

However, in the process of raising the stability level from the experimental version to the stable, there may be several breaking changes in the public API, and I am a little worried about whether it would be okay to raise the major version each that time. If Feign's major version has been changed several times with only changes to AsyncFeign, users might feel weird. I agree there will be complaints, but I think experimental imply that it could take a breaking change.

@velo velo merged commit 368818a into OpenFeign:master Oct 9, 2022
@wplong11 wplong11 deleted the 4-add-retry-feature-to-async-feign branch October 9, 2022 09:43
wplong11 added a commit to wplong11/feign that referenced this pull request Oct 9, 2022
The MethodInfo.configKey field is not more used since PR OpenFeign#1757 removing AsyncInvocation
velo added a commit that referenced this pull request Oct 11, 2022
The MethodInfo.configKey field is not more used since PR #1757 removing AsyncInvocation

Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>
wplong11 added a commit to wplong11/feign that referenced this pull request Oct 12, 2022
velo added a commit that referenced this pull request Oct 20, 2022
False negative test case is added in PR #1757

Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>
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

Successfully merging this pull request may close these issues.

Add retry feature to async feign
2 participants