-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
sample(04-grpc): add independent gRPC Client / Server sample respectively #13327
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 95db5acc-385b-40ba-8c36-c49c7e046909Details
💛 - Coveralls |
I wonder if adding yet another sample to this repository doesn't create even more unnecessary overhead that we already face with updating their dependencies (maintenance burden). Is there any way we could merge this example with the existing grpc one? If not, I'd recommend creating a separate repository instead |
7e9dc01
to
3ec5be0
Compare
|
3ec5be0
to
00c4047
Compare
sayHelloStreamReply(request: HelloRequest): Observable<HelloReply> { | ||
const hero$ = new ReplaySubject<HelloReply>(); | ||
|
||
if (request.name === '') { | ||
hero$.error( | ||
new RpcException({ | ||
code: grpc.status.INVALID_ARGUMENT, | ||
message: 'request missing required field: name', | ||
}), | ||
); | ||
|
||
return hero$.asObservable(); | ||
} | ||
|
||
for (let i = 0; i < this.REPLY_COUNT; i++) { | ||
hero$.next({ message: 'Hello ' + request.name }); | ||
} | ||
hero$.complete(); | ||
|
||
return hero$.asObservable(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this using a ReplaySubject? This can be written in multiple other ways without the need of a ReplaySubject. The most concise of which would be
sayHelloStreamReply({ name }: HelloRequest): Observable<HelloReply> {
if (!name) {
throw new RpcException({
code: status.INVALID_ARGUMENT,
message: 'request missing required field: name',
});
}
return of({ message: `Hello ${name}` }).pipe(repeat(this.REPLY_COUNT));
}
but even, so I would caution showing an example of streaming synchronous replies like this. When the list of emissions is sufficiently large this can block the event loop writing out all the messages not allowing anything else to occur. This often results in health check failures in environments like kubernetes. Ideally each emission is written out asynchronously which can be achieved with scheduled
and an async scheduler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the code review and suggestions.
I hope the sample code will greatly help developers using nest & grpc.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
https://github.com/youngkiu/nest/blob/sample/34-grpc-client-server/sample/34-grpc-client-server/README.md
Currently, NestJS Server produces different results.
This will be resolved when #13195 is released.