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

Support router-api annotations for gRPC services #912

Merged
merged 10 commits into from Jan 17, 2020

Conversation

idelpivnitskiy
Copy link
Member

@idelpivnitskiy idelpivnitskiy commented Jan 8, 2020

Motivation:

gRPC should support annotations that help to configure the execution
strategy per service or per route. Also, gRPC should compute the
effective execution strategy based on the main execution strategy
from the builder configuration and from the route configuration.

Modifications:

  • Parse annotations from route-api module;
  • Compute the difference between execution strategy on the builder
    and on the route;
  • Add tests to verify that offloading works correctly for different
    execution strategy configurations;
  • Add tests to verify that misconfiguration of route-api annotations
    will be explained to the user;
  • Adjust gRPC codegen for new features;
  • Change difference functions to compare with noOffloadsStrategy
    by configuration, not by reference;
  • Make NoOffloadsHttpExecutionStrategy no op;
  • Introduce RouteExecutionStrategyFactory and use it for grpc router;
  • Use RouteExecutionStrategyFactory in Jersey router;
  • Fix tests;

Result:

gRPC routes correctly compute per-route execution strategy
configuration.

Motivation:

gRPC should support annotations that help to configure
the execution strategy per service or per route. Also,
gRPC should compute the effective execution strategy
based on the main execution strategy from the builder
configuration and from the route configuration.

Modifications:

- Parse annotations from `route-api` module;
- Compute difference between execution strategy on
the builder and on the route;
- Add tests to verify that offloading works correctly
for different execution strategy configurations;
- Add tests to verify that misconfiguration of
`route-api` annotations will be explained to the user;
- Adjust gRPC codegen for new features;
- Fix tests;

Result:

gRPC routes correctly support per-route execution
strategy configuration.
return null;
}
if (left == noOffloadsStrategy()) {
if (noOffloads(left)) {
Copy link
Member Author

@idelpivnitskiy idelpivnitskiy Jan 8, 2020

Choose a reason for hiding this comment

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

We can not compare with noOffloadsStrategy() by reference, because we may receive a noOffloadsStrategy() which is wrapped by DefaultGrpcExecutionStrategy here or users may give their own implementation of the strategy without offloading.
@NiteshKant WDYT about introducing something like boolean HttpExecutionStrategy.isNoOffloading()? Or maybe even promoting this method to ExecutionStrategy interface. That will help us to avoid duplication of noOffloads method impl, we will be able to use it in DefaultHttpExecutionStrategy.offloadService in addition to diff == null check, and users will have an easy way to check that there is no offloading.

Copy link
Member

@NiteshKant NiteshKant Jan 9, 2020

Choose a reason for hiding this comment

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

I do not think that we will have many users checking presence/absence of no-offloads so I do not think we should promote this to a public API. The code repetition in jersey code is perhaps we can avoid by using this difference() method in jersey? There shouldn't be a reason for us to be different in behavior between routers. Is changing jersey to use the common difference() method here something you are thinking of doing?

Copy link
Member Author

@idelpivnitskiy idelpivnitskiy Jan 16, 2020

Choose a reason for hiding this comment

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

I see that the PR #682 changes jersey router to use its own impl of difference function instead of the HttpExecutionStrategies.difference. If I change EndpointEnhancingRequestFilter to use HttpExecutionStrategies.difference, some tests in jersey module fail.

Copy link
Member

@NiteshKant NiteshKant Jan 17, 2020

Choose a reason for hiding this comment

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

We can fix the tests if they are inaccurate or do not align with gRPC, there isn't a reason why they should be different. It is OK to do this in a follow up but we should look at reducing special cases between modules which makes it hard to reason about behavior later.

Copy link
Member Author

@idelpivnitskiy idelpivnitskiy Jan 17, 2020

Choose a reason for hiding this comment

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

Created an issue #924

public Single<TestResponse> test(final GrpcServiceContext ctx, final TestRequest request) {
final ThreadInfo threadInfo = new ThreadInfo(ctx);
return succeeded(threadInfo)
.afterOnSubscribe(__ -> threadInfo.responseOnSubscribeThreadName = threadName())
Copy link
Member Author

@idelpivnitskiy idelpivnitskiy Jan 8, 2020

Choose a reason for hiding this comment

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

I see that Single and Completable have before/afterOnSubscribe, but doesn't have whenOnSubscribe (but Publisher has it). If they were missed, I can add them in a follow-up or create a "good first issue".
@NiteshKant wdyt?

Also, we don't have whenSubscriber for all 3 async sources and Publisher.whenSubscription, but before/after* variants are exist.

Copy link
Member

@NiteshKant NiteshKant Jan 16, 2020

Choose a reason for hiding this comment

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

It may have got missed, creating a good first issue SGTM

Copy link
Member Author

@idelpivnitskiy idelpivnitskiy Jan 18, 2020

Choose a reason for hiding this comment

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

return null;
}
if (left == noOffloadsStrategy()) {
if (noOffloads(left)) {
Copy link
Member

@NiteshKant NiteshKant Jan 9, 2020

Choose a reason for hiding this comment

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

I do not think that we will have many users checking presence/absence of no-offloads so I do not think we should promote this to a public API. The code repetition in jersey code is perhaps we can avoid by using this difference() method in jersey? There shouldn't be a reason for us to be different in behavior between routers. Is changing jersey to use the common difference() method here something you are thinking of doing?

@idelpivnitskiy idelpivnitskiy requested a review from NiteshKant Jan 16, 2020
Copy link
Member

@NiteshKant NiteshKant left a comment

One comment then LGTM

Copy link
Member

@NiteshKant NiteshKant left a comment

💯

@idelpivnitskiy idelpivnitskiy merged commit 6bb2722 into apple:master Jan 17, 2020
3 checks passed
@idelpivnitskiy idelpivnitskiy deleted the grpc-annotations branch Jan 17, 2020
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.

None yet

2 participants