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.

Sign up for GitHub

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

Jump to bottom

Rich error model client api #1665

Merged
merged 2 commits into from
Dec 21, 2022
Merged

Rich error model client api #1665

merged 2 commits into from
Dec 21, 2022

Conversation

raboof
Copy link
Member

@raboof raboof commented Aug 24, 2022

Based on #1540 - see discussion there

@lightbend-cla-validator
Copy link

Hi @raboof,

Thank you for your contribution! We really value the time you've taken to put this together.

We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it.
Please review the new CLA and sign it before we proceed with reviewing this pull request:

https://www.lightbend.com/contribute/cla

@johanandren johanandren marked this pull request as ready for review November 14, 2022 17:54
@ennru
Copy link
Member

ennru commented Dec 1, 2022 

Error:  [11/14/2022 17:56:30.436] [pool-9-thread-6-ScalaTest-running-GrpcExceptionHandlerSpec] [akka.grpc.scaladsl.GrpcExceptionHandler(akka://Test)] Unhandled error: [null]
java.lang.NullPointerException
	at akka.grpc.scaladsl.GrpcExceptionHandlerSpec.<init>(GrpcExceptionHandlerSpec.scala:44)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)

@lightbend-cla-validator
Copy link

Hi @raboof,

Thank you for your contribution! We really value the time you've taken to put this together.

We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it.
Please review the new CLA and sign it before we proceed with reviewing this pull request:

https://www.lightbend.com/contribute/cla

1 similar comment
@lightbend-cla-validator
Copy link

Hi @raboof,

Thank you for your contribution! We really value the time you've taken to put this together.

We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it.
Please review the new CLA and sign it before we proceed with reviewing this pull request:

https://www.lightbend.com/contribute/cla

@johanandren
Copy link
Member

@ennru this is now passing all but link and cla validator (who is drunk)

ennru
ennru reviewed Dec 21, 2022
View reviewed changes
interop-tests/src/test/scala/akka/grpc/scaladsl/RichErrorModelNativeSpec.scala Outdated

import akka.NotUsed
import akka.actor.ActorSystem
import akka.grpc.internal.RichGrpcMetadataImpl
Copy link
Member

Choose a reason for hiding this comment

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

So the user will need to directly access an Akka gRPC internal class match against?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. If this is the way to go, we should probably make this class non-internal to avoid confusion.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, that's not nice, we should make it public API.

Copy link
Member

Choose a reason for hiding this comment

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

Added in d052ee7

ennru
ennru approved these changes Dec 21, 2022
View reviewed changes
Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

An API suggestion, LGTM for the rest of it.

runtime/src/main/scala/akka/grpc/javadsl/Metadata.scala
*/
@ApiMayChange
@DoNotInherit
trait RichMetadata extends Metadata {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to use a postfix instead for easier discoverability with code completion?
MetadataExtended, MetadataEnriched

Copy link
Member

Choose a reason for hiding this comment

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

Good idea

@johanandren
Copy link
Member

Would be good with less low level access to parsed values the predefined details types (like def help: Option[Help]) but the problem is that we don't want the dependency generated messages from core.

@johanandren johanandren merged commit 1c08d13 into main Dec 21, 2022
@johanandren johanandren deleted the rich-error-client branch December 21, 2022 13:29
@johanandren johanandren added this to the 2.3.0-M2 milestone Dec 21, 2022
@johanandren
Copy link
Member

Meh, I merged before pushed the rename. 🤦

Follow up PR coming.

@johanandren johanandren mentioned this pull request Dec 21, 2022
Merged
@jtjeferreira jtjeferreira mentioned this pull request Oct 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanandren johanandren johanandren left review comments

@sebastian-alfers sebastian-alfers sebastian-alfers left review comments

@ennru ennru ennru approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.3.0-M2
Development

Successfully merging this pull request may close these issues.
5 participants
@raboof @lightbend-cla-validator @ennru @johanandren @sebastian-alfers