feast-dev / feast-dev/feast

RemoteRegistry keeps no client-side cache, so allow_cache=True still issues one RPC per read, unlike every CachingRegistry implementation

Open
#6,672 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
7.3k
Forks
1.4k
Avg merge
1d 21h
Merged PRs (30d)
15

Description

## Expected Behavior

`allow_cache=True` on a registry read should mean the read may be served from a
cache without contacting the registry backend, for every registry
implementation. A caller passing `allow_cache=True` in a loop should not issue
one backend round-trip per iteration.

## Current Behavior

`RemoteRegistry` holds no client-side cache. Each read builds a request, calls
the stub and converts the response
(`sdk/python/feast/infra/registry/remote.py`, line 428 on `master`):

```python
def get_feature_view(
self, name: str, project: str, allow_cache: bool = False
) -> FeatureView:
request = RegistryServer_pb2.GetFeatureViewRequest(
name=name, project=project, allow_cache=allow_cache
)
response = self.stub.GetFeatureView(request)
return FeatureView.from_proto(response)
```

`allow_cache` is forwarded as a field on the request message. It selects whether
the *server* may answer from its cache; it has no effect on whether the client
issues the RPC. The pattern is uniform: on `master` the class has 28 methods
taking `allow_cache` and 50 `self.stub.(...)` call sites, and the word
"cache" appears in the file only as that passthrough parameter. No response is
ever retained.

`CachingRegistry` behaves differently for the same argument
(`caching_registry.py`, line 235):

```python
def get_stream_feature_view(
self, name: str, project: str, allow_cache: bool = False
) -> StreamFeatureView:
if allow_cache:
self._refresh_cached_registry_if_necessary()
return proto_registry_utils.get_stream_feature_view(
self.cached_registry_proto, name, project
)
return self._get_stream_feature_view(name, project)
```

Here `allow_cache=True` is an in-process lookup against
`cached_registry_proto`. `RemoteRegistry` is the outlier: every other registry
implementation has a client-side cache, whether inherited or hand-rolled —
`SqlRegistry(CachingRegistry)` (`sql.py:319`), and `Registry(BaseRegistry)`
(`registry.py:174`, the local-file/S3/GCS registry) and
`SnowflakeRegistry(BaseRegistry)` (`snowflake.py:125`), which both maintain their
own `cached_registry_proto` and `_refresh_cached_registry_if_necessary`.
`RemoteRegistry(BaseRegistry)` (`remote.py:97`) has neither.

The server does honour the field — `registry_server.py:410-418` forwards
`allow_cache=request.allow_cache` to the proxied registry — so for the remote
client `allow_cache` is strictly a server-side hint. It never suppresses the
call.

So the same argument means "may skip the backend" on four registries and
"always one full round-trip" on the fifth. `FeatureStore` methods that default
`allow_registry_cache=True` — `push` (lines 3183, 3221) and
`write_to_offline_store` (3435) — are written on the assumption that repeated
resolution is cheap; that assumption silently does not hold for the remote
registry. (`get_feature_view` and `get_entity` default to `False`, so this is
about the write/push paths specifically.)

**Observed impact.** Long-running batch jobs resolve feature view metadata per
batch. On a remote registry every such resolution is a gRPC round-trip even with
`allow_registry_cache=True`, so registry traffic scales with batch count. It also
removes the usual mitigation: holding one `FeatureStore` for the whole run does
not make the reads cheap, because there is no feature-view cache in the client to
warm.

Two things already in the tree are worth naming, since neither closes this:

- **A partial client cache exists, for feature services only.** `55c2f185`
("perf: Cache feature view resolution in get_online_features") added
`_feature_service_cache`, which `FeatureStore.__init__` attaches to the registry
instance by `setattr` (`feature_store.py:431-436`) and `utils.py:1190-1192`
reads. It memoises feature-service→feature-ref resolution only. `get_feature_view`
and the other object getters are untouched and still pay per call. Notably that
commit touched `base_registry.py`, `caching_registry.py`, `registry.py` and
`utils.py`, and deliberately not `remote.py`.
- **`is_cache_valid()` lets a caller probe, but only if they know to.**
`BaseRegistry.is_cache_valid` (`base_registry.py:1004`) returns `False` with the
docstring *"Registries without caching always return False (every read goes to
the backing store)"*; `CachingRegistry` overrides it (`caching_registry.py:509`)
and `RemoteRegistry` does not. So the divergence is discoverable at runtime.
That is an unadvertised probe, not a documented contract, and it does not help a
caller who simply wants `allow_cache` to mean one thing across registries.

#4710 reported the same shape of problem — repeated per-call metadata resolution
from the registry dominating `get_online_features`. It shows as closed/completed,
but it was closed by `stale[bot]` on 2025-05-08 with nothing landed at the time,
so it should not be read as a resolution. The caching that did land later
(`55c2f185`, above) skipped `remote.py`. In that thread a maintainer states the
premise of this issue directly:

> I guess with your wrapper you're overcoming the fact that RemoteRegistry
> doesn't extend CachingRegistry. (and even if it was the way CachingRegistry
> maintains a cache is less that ideal, but that's a different story)
>
> — @tokoko, #4710

## Steps to reproduce

1. Configure a remote registry:

```yaml
project: demo
registry:
registry_type: remote
path: registry.example:80
```

2. Read the same object repeatedly with the cache explicitly allowed:

```python
for _ in range(100):
store.get_feature_view("my_fv", allow_registry_cache=True)
```

3. Observe 100 `GetFeatureView` RPCs. Count them in the registry server's access
log, or wrap `store.registry.stub` and tally calls.

4. Run the same loop against a `registry_type: sql` registry. It issues at most
one backend read, then serves from `cached_registry_proto` until the TTL
expires.

### Specifications

- Version: 0.62.0; verified present on 0.65.0 (latest release) and `master`
- Platform: Linux, Python 3.12, Kubernetes; remote registry over gRPC
- Subsystem: registry — `feast.infra.registry.remote.RemoteRegistry`

## Possible Solution

Two directions; the first is the smaller change.

1. Make `RemoteRegistry` extend `CachingRegistry`, implementing the abstract
`_get_*` / `_list_*` hooks with the existing stub calls. The caching, TTL and
refresh behaviour then come from the shared base, and `allow_cache` acquires
the same meaning it already has everywhere else.

The refresh contract is already satisfied. `CachingRegistry.__init__` line 39
populates the cache with `self.cached_registry_proto = self.proto()`, and
`RemoteRegistry.proto` exists (`remote.py:706-707`,
`return self.stub.Proto(Empty())`) against the `Proto` RPC restored by #6558
(`RegistryServer.proto:100`, handler `registry_server.py:185`). So no
reassembly from `list_*` RPCs is needed — the one-shot refresh works as-is.

One scoping detail: 28 `RemoteRegistry` methods take `allow_cache` but
`CachingRegistry` declares 26 `_get_*`/`_list_*` hooks. `get_registry_lineage`
and `get_object_relationships` have no corresponding hook and would need
separate handling.

2. If unifying is too large, document the divergence — that `allow_cache` on the
remote registry is a server-side hint only and never suppresses the RPC — so
callers can size their own caching. `is_cache_valid()` returning `False`
already encodes this, but nothing points a caller at it; the registry docs say
nothing about which registry types honour `allow_cache` client-side, and the
shared signature implies all of them do.

Related: #6671, where this cost is multiplied — a write path issues a
guaranteed-miss lookup before the real one, so a remote registry pays two RPCs
per batch where a caching registry pays roughly none.

Contributor guide

Open the contributing guide

Research direction

Start with sdk/python/feast/infra/registry/remote.py, especially RemoteRegistry and its stub calls, then compare the cache and hook contracts in caching_registry.py and base_registry.py. Reproduce the repeated get_feature_view loop with a remote registry and inspect the existing registry implementations before choosing an approach. Done means allow_cache=True has consistent client-side behavior without breaking the lineage or object-relationship methods, and repeated reads no longer issue one RPC each.

Written by the indexing model from the issue text.

Assessment

Tech stack
grpc, python
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.