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

Add fallback stubfactory #413

Merged
merged 3 commits into from
Aug 24, 2020
Merged

Add fallback stubfactory #413

merged 3 commits into from
Aug 24, 2020

Conversation

ST-DDT
Copy link
Collaborator

@ST-DDT ST-DDT commented Aug 19, 2020 •
edited
Loading

Fixes #410

@asarkar I used the kotlin project to test this fix and it seems to be working as expected. Could you please try and confirm that it solves the problem for you as well?

  • Implement fix
  • Manually confirm that it "works for me"
  • Manually confirm that it "works for the issue reporter"
  • Add automated tests for it

@ST-DDT ST-DDT added bug Something does not work as expected high priority Should be fixed/implemented ASAP labels Aug 19, 2020
@ST-DDT ST-DDT added this to the 2.10.1 milestone Aug 19, 2020
@ST-DDT ST-DDT self-assigned this Aug 19, 2020
@asarkar
Copy link

asarkar commented Aug 20, 2020

Could you please try

Can you please upload a JAR here that you built locally? I don't want to go through cloning the repo and building it locally.

@ST-DDT
Copy link
Collaborator Author

ST-DDT commented Aug 20, 2020

Could you please try

Can you please upload a JAR here that you built locally? I don't want to go through cloning the repo and building it locally.

Sure grpc-client-spring-boot-autoconfigure 2.10.1-SNAPSHOT.zip

@asarkar
Copy link

asarkar commented Aug 20, 2020

Seems to be working with the example project. I see that you have left the other StubFactory implementation classes; isn't the FallbackStubFactory going to replace them all? It probably should be called DefaultStubFactory.

@ST-DDT
Copy link
Collaborator Author

ST-DDT commented Aug 20, 2020

Seems to be working with the example project. I see that you have left the other StubFactory implementation classes; isn't the FallbackStubFactory going to replace them all? It probably should be called DefaultStubFactory.

I have issues with managing the order of them. Setting @Order(HIGHEST/LOWEST) doesn't seem to have any impact on them.
Thats why I kept them as is for now.

Also I'm not sure whether I should cache the factory methods/constructors for subsequent calls.

@ST-DDT ST-DDT marked this pull request as ready for review August 22, 2020 17:15
@ST-DDT ST-DDT requested a review from yidongnan August 22, 2020 17:16
@ST-DDT
Copy link
Collaborator Author

ST-DDT commented Aug 22, 2020

@yidongnan Please create a release once this PR and #417 is merged.

yidongnan
yidongnan approved these changes Aug 24, 2020
View reviewed changes
Copy link
Collaborator

@yidongnan yidongnan left a comment

Choose a reason for hiding this comment

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

LGTM

@yidongnan yidongnan merged commit 7734df3 into master Aug 24, 2020
@yidongnan yidongnan deleted the fix/stub-fallback branch August 24, 2020 03:00
@yidongnan
Copy link
Collaborator

@ST-DDT 2.10.1 just released

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

@yidongnan yidongnan yidongnan approved these changes

Assignees

@ST-DDT ST-DDT

Labels
bug Something does not work as expected high priority Should be fixed/implemented ASAP
Projects
None yet
Milestone
2.10.1
Development

Successfully merging this pull request may close these issues.

Stub creation has no fallback
3 participants
@ST-DDT @asarkar @yidongnan