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

Remove unused internal code from servicetalk-grpc-api #2299

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

#1893 removed gRPC filter functionality, but left some internal code
required to support removed filters.

Modifications:

  • Remove GrpcRoutes#drainToStreamingRoutes();
  • Remove GrpcRouteConversions, remove conversions from RouteProvider;
  • Make RouteProvider class private;
  • Remove unused GrpcUtils#newResponse overload;

Result:

No unused code.

Motivation:

apple#1893 removed gRPC filter functionality, but left some internal code
required to support removed filters.

Modifications:

- Remove `GrpcRoutes#drainToStreamingRoutes()`;
- Remove `GrpcRouteConversions`, remove conversions from `RouteProvider`;
- Make `RouteProvider` class private;
- Remove unused `GrpcUtils#newResponse` overload;

Result:

No unused code.
@idelpivnitskiy idelpivnitskiy self-assigned this Jul 29, 2022
Copy link
Member

@Scottmitch Scottmitch left a comment

Choose a reason for hiding this comment

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

looks good! just a few suggestions.

@@ -952,5 +849,31 @@ public Completable closeAsync() {
public Completable closeAsyncGracefully() {
return closeable.closeAsyncGracefully();
}

private static AsyncCloseable toAsyncCloseable(final GracefulAutoCloseable original) {
Copy link
Member

Choose a reason for hiding this comment

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

did you consider putting this someone more visible (AutoClosableUtils, AsyncCloseables, ..)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do it in a separate PR for visibility and will use the same utility in HTTP conversions

Copy link
Contributor

@bondolo bondolo left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

@idelpivnitskiy idelpivnitskiy merged commit 44c4343 into apple:main Jul 29, 2022
@idelpivnitskiy idelpivnitskiy deleted the dead-code branch July 29, 2022 15:55
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.

3 participants