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

Allow a user choose a path when a Thrift/gRPC service has more than one path #3024

Merged
merged 11 commits into from Oct 1, 2020

Conversation

kojilin
Copy link
Member

@kojilin kojilin commented Aug 25, 2020

Make doc service can let users choose a path when RPC service has more than two paths.

  • Make docs-client show thrift/grpc supported mime type endpoints in dropdown list.

Fix #2167

thrift

@CLAassistant
Copy link

CLAassistant commented Aug 25, 2020

CLA assistant check
All committers have signed the CLA.

@kojilin kojilin closed this Aug 25, 2020
@kojilin kojilin deleted the thrift-example branch August 25, 2020 10:04
@kojilin kojilin restored the thrift-example branch August 25, 2020 10:04
@kojilin kojilin reopened this Aug 25, 2020
@kojilin kojilin force-pushed the thrift-example branch 2 times, most recently from c27abd2 to 1cb11d4 Compare August 25, 2020 10:10
@codecov
Copy link

codecov bot commented Aug 25, 2020

Codecov Report

Merging #3024 into master will increase coverage by 0.02%.
The diff coverage is 95.83%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3024      +/-   ##
============================================
+ Coverage     73.38%   73.41%   +0.02%     
- Complexity    12288    12305      +17     
============================================
  Files          1066     1066              
  Lines         47593    47634      +41     
  Branches       6003     6009       +6     
============================================
+ Hits          34927    34969      +42     
+ Misses         9617     9612       -5     
- Partials       3049     3053       +4     
Impacted Files Coverage Δ Complexity Δ
...a/com/linecorp/armeria/server/docs/MethodInfo.java 72.85% <ø> (-0.76%) 19.00 <0.00> (-1.00)
...e/armeria/server/annotated/PathPatternService.java 80.00% <0.00%> (-20.00%) 4.00 <0.00> (ø)
...ria/internal/server/grpc/GrpcDocServicePlugin.java 88.88% <ø> (ø) 53.00 <0.00> (ø)
...a/com/linecorp/armeria/server/docs/DocService.java 93.61% <100.00%> (+0.03%) 42.00 <1.00> (ø)
...in/java/example/armeria/server/annotated/Main.java 58.33% <100.00%> (+10.96%) 2.00 <0.00> (ø)
.../grpc/src/main/java/example/armeria/grpc/Main.java 70.96% <100.00%> (+0.96%) 2.00 <0.00> (ø)
...ift/src/main/java/example/armeria/thrift/Main.java 64.00% <100.00%> (+1.50%) 2.00 <0.00> (ø)
...internal/server/thrift/ThriftDocServicePlugin.java 86.01% <100.00%> (+0.24%) 65.00 <0.00> (ø)
...inecorp/armeria/common/ClosedSessionException.java 54.54% <0.00%> (-18.19%) 5.00% <0.00%> (-1.00%)
...rmeria/internal/common/kotlin/ArmeriaKotlinUtil.kt 50.00% <0.00%> (-10.00%) 2.00% <0.00%> (-1.00%)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4839d78...0ea348f. Read the comment docs.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot! @kojilin

@kojilin
Copy link
Member Author

kojilin commented Aug 27, 2020

Let me do double check again, looks like few edge cases(grpc multiple paths) may have a problem :)
Looks like grpc need to use prefix: and I forget it.

Just curious, grpc only supports prefix, but thrift is prefix and exact, right?

@kojilin
Copy link
Member Author

kojilin commented Aug 27, 2020

Updated, ready for review again. I added some examples in the example project.

@minwoox
Copy link
Member

minwoox commented Aug 27, 2020

Could you invalidate and disable the TextField for additionalPath when it's Thrift or gRPC?

@kojilin
Copy link
Member Author

kojilin commented Aug 27, 2020

also query?

@minwoox
Copy link
Member

minwoox commented Aug 27, 2020

also query?

Yes. 😊

@kojilin
Copy link
Member Author

kojilin commented Sep 3, 2020

Updated.

  1. Make submit async and adding a new flag to avoid debug response cleaning by URL change
  2. Make endpoints page support rpc/annotated http service separately(Only use drop down to select)
  3. Move path check logic to transport, and make it return error instead of throw
  4. Disable query when it's rpc.

I tried to avoid changing dramatically, maybe there is a better way to achieve 1 and 2. ^^;;;

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

It looks like examplePaths is used for the candidate paths for RPC services. Is this a correct approach? They are not example strictly speaking. How about exposing multiple EndpointInfos in a MethodInfo instead? 🤔

docs-client/src/containers/MethodPage/EndpointPath.tsx Outdated Show resolved Hide resolved
@kojilin
Copy link
Member Author

kojilin commented Sep 7, 2020

It looks like examplePaths is used for the candidate paths for RPC services. Is this a correct approach? They are not example strictly speaking. How about exposing multiple EndpointInfos in a MethodInfo instead? 🤔

I'm also thought about this and originally thought it's ok to tread RPC exact path as an example and it's not that wrong to set the default for them.

So one is to let the user add an example path for RPC manually, but I think it's not intuitive.
Looks like using method info's endpoint directly and totally separate logic(Though it's already using isAnnotatedService already.) of client-side of endpoint module (tsx) is better and I will change this PR.

@kojilin
Copy link
Member Author

kojilin commented Sep 7, 2020

updated, using supported endpointInfos in methodinfo when method is rpc.

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Excellent work as usual, @kojilin. Sorry for a late review. 🙇

@trustin trustin added this to the 1.1.0 milestone Sep 20, 2020
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks again! @kojilin

docs-client/src/lib/transports/transport.ts Outdated Show resolved Hide resolved
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thank you!

@trustin trustin modified the milestones: 1.1.0, 1.2.0 Sep 22, 2020
@trustin trustin changed the title Allow a user choose a path when a Thrift/gRPC service has more than two paths Allow a user choose a path when a Thrift/gRPC service has more than one path Oct 1, 2020
@trustin trustin merged commit 5dea821 into line:master Oct 1, 2020
@trustin
Copy link
Member

trustin commented Oct 1, 2020

Thanks a lot, @kojilin. Would you mind updating the PR description so it reflects the changes made since the initial work?

@kojilin kojilin deleted the thrift-example branch October 1, 2020 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow a user choose a path when a Thrift/gRPC service has more than two paths.
5 participants