Skip to content

fix: Add gRPC timeout and keepalive to RemoteRegistry - #6698

Open
Arthur-plg wants to merge 3 commits into
feast-dev:masterfrom
Arthur-plg:fix/remote-registry-grpc-timeout
Open

fix: Add gRPC timeout and keepalive to RemoteRegistry#6698
Arthur-plg wants to merge 3 commits into
feast-dev:masterfrom
Arthur-plg:fix/remote-registry-grpc-timeout

Conversation

@Arthur-plg

@Arthur-plg Arthur-plg commented Aug 2, 2026 •
edited
Loading

Copy link
Copy Markdown

What this PR does / why we need it:

Configures a default deadline (timeout) and transport keepalive options (keepalive_time_ms, keepalive_timeout_ms) for the client-side gRPC channel in RemoteRegistry.

Without these configurations, network drops or silent firewall cuts (blackholes) after a TCP connection is established cause all registry calls to hang indefinitely in epoll_wait without raising any errors or timeouts.

Which issue(s) this PR fixes:

Fixes #6665

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

Misc

NONE

@Arthur-plg
Arthur-plg requested a review from a team as a code owner August 2, 2026 19:35
franciscojavierarceo
franciscojavierarceo requested changes Aug 3, 2026
View reviewed changes

@franciscojavierarceo franciscojavierarceo 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.

The new tuning fields are unconstrained Optional[int] values. timeout=0 or a negative keepalive interval/timeout is accepted by RemoteRegistryConfig and only fails later when gRPC constructs or uses the channel, turning a config typo into a runtime startup failure. We should validate positive values at config load (and add tests for zero/negative inputs); if None is intended to mean no deadline, the docs should also avoid calling this a default timeout.

@Arthur-plg

Arthur-plg commented Aug 3, 2026

Copy link
Copy Markdown
Author

Thanks for the feedback! I've added a Pydantic field_validator to RemoteRegistryConfig to ensure that timeout, keepalive_time_ms, and keepalive_timeout_ms are strictly positive at config load time.
I've also added unit tests validating that zero/negative inputs throw a ValidationError, and updated the documentation to clarify that calls will have no deadline if these values are not set.

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

Reviewers

@franciscojavierarceo franciscojavierarceo Awaiting requested review from franciscojavierarceo

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remote registry gRPC client sets no deadline and no keepalive, and neither is configurable: a blackholed connection hangs the caller indefinitely

3 participants

@Arthur-plg @franciscojavierarceo @apelong