Skip to content

Add option for gRPC client connection retries#270

Merged
DeRauk merged 4 commits intocoinbase:masterfrom
cultureamp:channel-args
Feb 5, 2024
Merged

Add option for gRPC client connection retries#270
DeRauk merged 4 commits intocoinbase:masterfrom
cultureamp:channel-args

Conversation

@hughevans
Copy link
Contributor

@hughevans hughevans commented Oct 17, 2023

Add a config option for a basic gRPC connection retry policy:

config.connection_options = {
  retry_connection: true
}

That uses a basic policy:

{
  retryableStatusCodes: ["UNAVAILABLE"],
  maxAttempts: 3,
  initialBackoff: "0.1s",
  backoffMultiplier: 2.0,
  maxBackoff: "0.3s"
}

There’s also a retry_policy option to set a custom policy if you want more control:

config.connection_options = {
  retry_policy: {
    retryableStatusCodes: ["UNAVAILABLE", "INTERNAL"],
    maxAttempts: 5,
    initialBackoff: "0.1s",
    backoffMultiplier: 2.5,
    maxBackoff: "0.7s"
  }
}

@hughevans hughevans changed the title Allow passing channel args to GRPC connection Allow passing channel args to gRPC connection Oct 17, 2023
@hughevans hughevans closed this Oct 18, 2023
@hughevans hughevans reopened this Nov 29, 2023
@hughevans hughevans changed the title Allow passing channel args to gRPC connection Add option for gRPC client connection retries Nov 29, 2023
maxAttempts: 3,
initialBackoff: "0.1s",
backoffMultiplier: 2.0,
maxBackoff: "0.3s"
Copy link
Contributor

Choose a reason for hiding this comment

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

@hughevans thanks for the PR! It looks good overall.

For maxAttempts, initialBackoff, backoffMultiplier, and maxBackoff - I think these are sane defaults, but if we're going to support retries I'd prefer that users be able to pass in their own values to override these if they want. Could you make them configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DeRauk Thank you for checking this out. I’ve gone ahead and added a retry_policy option there too.

Also I looked at the README, but was a bit unsure where exactly documentation for this should go logically. Is that something you could take on after merging possibly? 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, and sure.

end

context "when passing a custom retry policy" do
subject { Temporal::Connection::GRPC.new(nil, nil, identity, :this_channel_is_insecure, retry_policy:) }
Copy link
Contributor

Choose a reason for hiding this comment

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

@hughevans typo with retry_policy:)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry that only works in newer versions of Ruby. I have pushed a fix now thanks.

@DeRauk DeRauk merged commit 65dfdb0 into coinbase:master Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants