| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
The per-key running maximum is applied consistently in both sync and async write paths, and the added cases cover descending, ascending, unordered, and pre-existing stored timestamps. I found no blocking issue in the remote diff.
Sorry, something went wrong.
|
⚠️ Please install the Codecov Report❌ Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## master #6656 +/- ##
==========================================
+ Coverage 45.98% 46.06% +0.07%
==========================================
Files 414 414
Lines 50037 50045 +8
Branches 7147 7147
==========================================
+ Hits 23012 23051 +39
+ Misses 25413 25380 -33
- Partials 1612 1614 +2
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
online_write_batch reads every existing event timestamp in a single pipeline before it queues any write, so rows in the same batch that share an entity key all compared against the same pre-batch snapshot. None of them could see the others, so every row passed the staleness check and Redis applied the queued writes in list order — the last row won regardless of its event time. A batch of [t3, t2, t1] for one entity key left the value belonging to t1 in the store. Track the latest event timestamp queued so far for each entity key and compare each row against whichever is newer: the stored timestamp, or an earlier row of the same batch. Batches that already arrived in ascending order keep their existing behaviour, and rows older than the value already stored are still skipped. online_write_batch_async had the identical defect and gets the same fix. The skip_dedup fast path is deliberately left alone, since it documents that out-of-order writes are not a concern when it is enabled. Fixes feast-dev#5163 Signed-off-by: adarshsm <24850536+adarshsm@users.noreply.github.com>
| Back | FazBrowse Home | New Git URL |
What this PR does / why we need it:
online_write_batch performs its staleness check against a snapshot that is read
before any write is queued:
pipe.execute() once.
otherwise queue an hset.
Because the read phase completes before the first write is queued, rows in the same batch
that share an entity key all compare against the same pre-batch value. None of them can
see the others. When nothing is stored yet, prev_event_time is None for all of them,
the guard is skipped entirely, and every row gets an hset queued. Redis applies
pipelined commands in order, so the last row in list order wins — regardless of its
event time.
Pushing one entity key with timestamps in descending order therefore leaves the oldest
value in the store:
No error and no warning — just a silently incorrect feature value.
The fix
Track the latest event timestamp queued so far per entity key within the batch, and
compare each row against whichever is newer: the stored timestamp, or an earlier row of
the same batch.
redis_key_bin is already computed in the read phase and uniquely identifies
(project, entity_key), so it was free to reuse as the dedup key. The guard had to move
out of the if prev_event_time: block, with prev_total_nanos initialised to 0 so the
max() works when nothing is stored yet — that's the structural part of the diff.
online_write_batch_async had the identical defect and gets the same fix; fixing only the
sync path would have left half the bug in place.
The skip_dedup fast path is deliberately untouched. Its own comment says it is "suitable
for initial loads or append-only pipelines where out-of-order writes are not a concern," so
losing ordering there is the documented tradeoff of enabling it.
One design note worth your input
The issue suggests sorting the batch by timestamp, or reducing to one row per key. Either
would work, but both change behaviour for batches that are already correct — they alter
how many writes get queued in the ascending case and shift the progress() accounting.
I went with the running-max approach because it repairs only the broken case and leaves
every already-working case byte-identical. Happy to switch to sort-or-reduce if you'd
rather have the write-amplification reduction too; it's a small change from here.
Behaviour that is intentionally preserved:
instant" semantics.
truthy prev_total_nanos, and max(0, ...) preserves that.
Note on the labels
The wontfix label on #5163 was applied by stale[bot] as its configured staleLabel,
not by a maintainer declining the report — the timeline shows the bot's comment as the only
activity on the issue. Flagging it so it doesn't read as a prior decision. kind/bug and
priority/p2 are the defaults from bug_report.md. If this is out of scope for other
reasons, I'm glad to hear it.
Which issue(s) this PR fixes:
Fixes #5163
Checks
Testing Strategy
Misc
Five tests added to sdk/python/tests/unit/infra/online_store/test_redis.py, reusing the
existing MagicMock pipeline idiom, so no Redis server or Docker is needed:
The two that pass before the fix are the load-bearing ones — they're there to show the
change repairs the broken orderings without disturbing the ascending case or the existing
staleness guard.
The pre-fix failure is assert b'\x18\n' == b'\x18\x1e', i.e. int32_val=10 where 30 was
expected — the reporter's "returns 10 instead of 30", reproduced deterministically.
test_redis.py goes 18 → 23 passing. ruff check, ruff format --check and
mypy feast/infra/online_stores/redis.py are all clean.
I ran unit tests only, not the integration suite — it needs cloud credentials I don't have.
Happy to add an integration test if you'd like one for this path.
One small correction to the issue for the record: it describes the code as skipping
"records with timestamps older than what's already been processed in the current batch."
Nothing intra-batch is tracked today, which is exactly why the bug exists — but the symptom
as reported is accurate.