Skip to content
This repository was archived by the owner on Mar 13, 2021. It is now read-only.

Migrate to grpc-js#181

Merged
fbiville merged 1 commit into
projectriff:masterfrom
fbiville:issue_113
Apr 20, 2020
Merged

Migrate to grpc-js#181
fbiville merged 1 commit into
projectriff:masterfrom
fbiville:issue_113

Conversation

@fbiville

@fbiville fbiville commented Apr 9, 2020

Copy link
Copy Markdown
Contributor

Fixes #113

@fbiville fbiville force-pushed the issue_113 branch 2 times, most recently from 2e05e2a to e8f46f4 Compare April 9, 2020 13:40
@fbiville

fbiville commented Apr 9, 2020

Copy link
Copy Markdown
Contributor Author

Biggest changes:

  • proto has to be loaded at runtime
  • plain old Javascript objects are generated for the gRPC data types (no accessor methods generated)
  • gRPC maps are generated as an plain old objects
  • server.bind -> server.bindAsync
  • cancelWithStatus -> sendError

Possible source of worry (i.e. investigation needed):

@scothis scothis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you have a sense of the performance impact? if any

@fbiville

fbiville commented Apr 9, 2020

Copy link
Copy Markdown
Contributor Author

I managed to successfully deploy and run the function on the core runtime (I have a strange issue with Knative).

@fbiville fbiville marked this pull request as ready for review April 9, 2020 16:36
@fbiville fbiville requested a review from scothis April 9, 2020 16:36
@fbiville

fbiville commented Apr 9, 2020

Copy link
Copy Markdown
Contributor Author

Do you have a sense of the performance impact? if any

I have spent a significant time getting a proper riff install on a PKS cluster, so I have not started looking at this particular aspect yet.

The Docker image fbiville/builder is available for consumption FYI.

@murgatroid99

Copy link
Copy Markdown

Hi,
I'm the grpc maintainer; I got here following the link from that issue about server.bind and I realized that I could help you simplify this migration. First, you can actually continue using the same proto Message classes you are currently using. The most recent version of grpc-tools has an option "generate_package_definition" that makes it generate a file that outputs the same kind of object that protoLoader.load outputs, so you can pass that into grpc.loadPackageDefinition. Using that option just requires changing part of your grpc-tools invocation like this: --grpc_out=generate_package_definition:./codegen.

Second, this doesn't necessarily help with this migration exactly but it explains why this part changed: the intended way of having a server end a call with a non-OK status is to call stream.emit('error', status). Seeing your code I realize that that's not obvious and it would be better to have something like stream.endWithStatus(statusObject).

@fbiville

fbiville commented Apr 20, 2020

Copy link
Copy Markdown
Contributor Author

@murgatroid99 thanks for the feedback (apologies for the delay, I was in "staycation" until today).

Regarding the emission of errors, does this mean that:

grpcStream.call.sendError({
    code: grpc.status.INVALID_ARGUMENT,
    details: `${errors.reserved_prefixes.NOT_ACCEPTABLE}: ${err.cause}`,
});

can be replaced with:

grpcStream.emit('error', {
    code: grpc.status.INVALID_ARGUMENT,
    details: `${errors.reserved_prefixes.NOT_ACCEPTABLE}: ${err.cause}`,
});

?

I tried to follow your advice and open another draft PR, however I now face several issues:

@fbiville fbiville marked this pull request as draft April 20, 2020 10:14
@fbiville

Copy link
Copy Markdown
Contributor Author

Converting this PR to draft. I'll try to open another one with aforementioned suggestions.

@fbiville fbiville marked this pull request as ready for review April 20, 2020 15:44
@murgatroid99

Copy link
Copy Markdown

@fbiville That is what I meant about emitting errors.

I don't see anything wrong with how you're importing the package definition and I don't know why it would be returning UNIMPLEMENTED errors.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate node function on PKS crash loop

3 participants