Skip to content

fix: Stop the Couchbase online store reporting failed writes as written - #6911

Merged
ntkathole merged 1 commit into
feast-dev:masterfrom
ericdelorefice:couchbase-write-errors
Oct 1, 2026
Merged

ntkathole merged 1 commit into
feast-dev:masterfrom
ericdelorefice:couchbase-write-errors

Conversation

@ericdelorefice

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Problem

CouchbaseOnlineStore.online_write_batch wraps each upsert in except Exception, logs it, and carries on. progress(1) still runs for the row that failed. So when Couchbase times out or rejects a write, materialization finishes normally, reports every row as written, and the online store is missing those rows. update() does the same when a scope or collection can't be created, so feast apply also succeeds against a store that isn't set up.

The rest of the file already draws the line in the right place. online_read catches only DocumentNotFoundException, and update() already catches ScopeAlreadyExistsException and CollectionAlreadyExistsException on their own. Of the other 22 online stores, 21 have no handler around their writes and MySQL catches and re-raises, so a failed write reaches the caller everywhere else.

Reproduction

A mocked collection whose upsert raises TimeoutException, on master. online_write_batch with three rows logs three errors and returns normally:

ERROR: Error upserting document test_project:driver_stats:...:trips: TimeoutException(<message=upsert timed out>)
ERROR: Error upserting document test_project:driver_stats:...:trips: TimeoutException(<message=upsert timed out>)
ERROR: Error upserting document test_project:driver_stats:...:trips: TimeoutException(<message=upsert timed out>)

No exception reaches the caller, and progress is called three times.

Change

The three generic handlers keep their log line and raise. The "already exists" handlers and the read path are unchanged, so re-running feast apply against an existing scope and collection still works.

Verification

New tests/unit/infra/online_store/test_couchbase_online_store.py, 5 tests. With the source reverted to master and the tests kept, 3 fail: the write, the scope and the collection cases. The 2 that pass are the controls, a normal write and an already-existing scope and collection, which behave the same before and after.

The rest of tests/unit/infra/online_store gives the same result with and without this change. ruff check and ruff format --check are clean.

Which issue(s) this PR fixes:

None filed. The reproduction is above.

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

The store is marked alpha, so I kept this to the error handling and left everything else alone.

@ericdelorefice
ericdelorefice requested a review from a team as a code owner September 30, 2026 19:48
@codecov-commenter

codecov-commenter commented Oct 1, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.19%. Comparing base (dd9f099) to head (4cdfc7f).
⚠️ Report is 1 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6911      +/-   ##
==========================================
+ Coverage   48.04%   48.19%   +0.14%     
==========================================
  Files         427      427              
  Lines       53591    53594       +3     
  Branches     7800     7800              
==========================================
+ Hits        25749    25827      +78     
+ Misses      25986    25908      -78     
- Partials     1856     1859       +3     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 49.53% <100.00%> (+0.15%) ⬆️
Files with missing lines Coverage Δ
.../online_stores/couchbase_online_store/couchbase.py 60.93% <100.00%> (+60.93%) ⬆️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 023614b...4cdfc7f. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

online_write_batch logged and skipped any upsert that raised, and still
called progress for it, so materialization succeeded with rows missing
from the online store. update() did the same when a scope or collection
could not be created. The generic handlers now log and re-raise; the
already-exists handlers are unchanged.

Signed-off-by: Eric Del Orefice <114540347+ericdelorefice@users.noreply.github.com>
@ntkathole
ntkathole force-pushed the couchbase-write-errors branch from 4cdfc7f to f096b89 Compare October 1, 2026 04:33
@ntkathole
ntkathole merged commit b3f3c98 into feast-dev:master Oct 1, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants