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

Support setting deadline per-call #1838

Merged
merged 6 commits into from
Oct 14, 2023
Merged

Support setting deadline per-call #1838

merged 6 commits into from
Oct 14, 2023

Conversation

longshorej
Copy link
Contributor

Adds a setDeadline method to the request builder, so that it can optionally be specified per call.

@lightbend-cla-validator
Copy link

Hi @longshorej,

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

@lightbend-cla-validator
Copy link

Hi @longshorej,

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 reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Nice! Will need Java DSL API in akka.grpc.javadsl.SingleResponseRequestBuilder as well (using Java duration).

@longshorej
Copy link
Contributor Author

Nice! Will need Java DSL API in akka.grpc.javadsl.SingleResponseRequestBuilder as well (using Java duration).

Ah, got it. I will add this sometime in the next couple days. Thanks.

@longshorej
Copy link
Contributor Author

Hi @johanandren, I added the Java API methods, as well as streaming for Java+Scala.

patriknw
patriknw reviewed Oct 2, 2023
View reviewed changes
Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

looking good

runtime/src/main/scala/akka/grpc/scaladsl/RequestBuilder.scala
* Set the deadline for this call
* @return A new request builder, that will use the supplied deadline when invoked
*/
def setDeadline(deadline: Duration): SingleResponseRequestBuilder[Req, Res]
Copy link
Member

Choose a reason for hiding this comment

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

Should we use FiniteDuration here instead. Always somewhat confusing what non finite Duration means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went to make this change but found that GrpcClientSettings#deadline is already typed as Duration and supports supplying infinite, which means no timeout -- in NettyClientUtils it's implemented as follows:

  @InternalApi private[akka] def callOptionsWithDeadline(
      defaultOptions: CallOptions,
      settings: GrpcClientSettings): CallOptions =
    settings.deadline match {
      case d: FiniteDuration => defaultOptions.withDeadlineAfter(d.toMillis, TimeUnit.MILLISECONDS)
      case _                 => defaultOptions
    }

...so, I updated the call sites to check isFinite/null (Scala/Java) and clear timeout if its infinite, which I think is more consistent with how the settings are parsed. Let me know what you think though - happy to make it FiniteDuration if you think that's better.

Copy link
Member

Choose a reason for hiding this comment

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

good, thanks

runtime/src/main/scala/akka/grpc/internal/RequestBuilderImpl.scala Outdated
new ScalaBidirectionalStreamingRequestBuilder[I, O](
descriptor,
channel,
defaultOptions.withDeadlineAfter(deadline.toMillis, TimeUnit.MILLISECONDS),
Copy link
Member

Choose a reason for hiding this comment

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

see my comment about FiniteDuration, this will probably fail otherwise for some non finite durations

@patriknw
Copy link
Member

patriknw commented Oct 2, 2023

could you also add some mima filters:

[error] akka-grpc-runtime: Failed binary compatibility check against com.lightbend.akka.grpc:akka-grpc-runtime_2.13:2.3.4! Found 4 potential problems (filtered 2)
[error]  * abstract method setDeadline(java.time.Duration)akka.grpc.javadsl.SingleResponseRequestBuilder in interface akka.grpc.javadsl.SingleResponseRequestBuilder is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.grpc.javadsl.SingleResponseRequestBuilder.setDeadline")
[error]  * abstract method setDeadline(java.time.Duration)akka.grpc.javadsl.StreamResponseRequestBuilder in interface akka.grpc.javadsl.StreamResponseRequestBuilder is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.grpc.javadsl.StreamResponseRequestBuilder.setDeadline")
[error]  * abstract method setDeadline(scala.concurrent.duration.Duration)akka.grpc.scaladsl.SingleResponseRequestBuilder in interface akka.grpc.scaladsl.SingleResponseRequestBuilder is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.grpc.scaladsl.SingleResponseRequestBuilder.setDeadline")
[error]  * abstract method setDeadline(scala.concurrent.duration.Duration)akka.grpc.scaladsl.StreamResponseRequestBuilder in interface akka.grpc.scaladsl.StreamResponseRequestBuilder is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.grpc.scaladsl.StreamResponseRequestBuilder.setDeadline")
[error] java.lang.RuntimeException: Failed binary compatibility check against com.lightbend.akka.grpc:akka-grpc-runtime_2.13:2.3.4! Found 4 potential problems (filtered 2)

@longshorej longshorej changed the title Permit setting deadline per-call Support setting deadline per-call Oct 4, 2023
@longshorej
Copy link
Contributor Author

could you also add some mima filters:

Added

patriknw
patriknw approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM

runtime/src/main/scala/akka/grpc/scaladsl/RequestBuilder.scala
* Set the deadline for this call
* @return A new request builder, that will use the supplied deadline when invoked
*/
def setDeadline(deadline: Duration): SingleResponseRequestBuilder[Req, Res]
Copy link
Member

Choose a reason for hiding this comment

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

good, thanks

@patriknw
Copy link
Member

sorry, we released a milestone, so latest is 2.4.0-M1
Can you move the mima filter to 2.4.0-M1, please.

@longshorej
Permit setting deadline per-call
677d24f 
Adds a setDeadline method to the request builder, so that it can optionally be
specified per call.
@longshorej
streaming
8bb2f4a
@longshorej
java
c8fd5bf
@longshorej
support clearing deadline
34ab47a
@longshorej
mima
9ec05cb
@longshorej
move mima filter to 2.4.0-M1
e1887a1
@longshorej
Copy link
Contributor Author

sorry, we released a milestone, so latest is 2.4.0-M1
Can you move the mima filter to 2.4.0-M1, please.

Done!

@johanandren johanandren enabled auto-merge (squash) October 14, 2023 11:12
@johanandren johanandren merged commit cdd9e63 into akka:main Oct 14, 2023
12 checks passed
@johanandren johanandren added this to the 2.4.0 milestone Oct 15, 2023
@patriknw
Copy link
Member

Thanks

johanandren pushed a commit that referenced this pull request Dec 14, 2023
@longshorej @johanandren
feat: Support setting deadline per-call (#1838)
5dce16a 
Adds a setDeadline method to the request builder, so that it can optionally be
specified per call.
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

@patriknw patriknw patriknw approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@longshorej @lightbend-cla-validator @patriknw @johanandren