chore: Add persistence store support for FDv2 #357
Conversation
Bug: Deadlock Risk in FeatureStoreClientWrapperIn Additional Locations (1) |
| self.__lock.unlock() | ||
|
|
||
| if modified: | ||
| self.__listeners.notify(status) |
There was a problem hiding this comment.
What is the threading situation like for this dispatch? Is the caller thread always the same thread?
There was a problem hiding this comment.
Yes, unfortunately that's the way the current listeners work in python. The notify method itself catches all exceptions, which is why we aren't doing it here.
There was a problem hiding this comment.
Ok. Well it is good in a sense, because it helps prevent out of order handling from unfortunate thread context switching. If it was multi-threaded, then I would be concerned we would need some async queue type situation.
| """ | ||
| update_status is called from the data store to push a status update. | ||
| """ | ||
| self.__lock.lock() |
There was a problem hiding this comment.
Is there a reason this has to use lock/unlock vs with?
There was a problem hiding this comment.
This is the LD provided RW lock. There is no context usage for this class.
keelerm84
force-pushed
the
mk/sdk-1504/persistence
branch
from
4e44909 to
e88a705
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| self.__status = status | ||
| modified = True | ||
|
|
||
| self.__lock.unlock() |
There was a problem hiding this comment.
Bug: Deadlock Risk in FeatureStoreClientWrapper
In FeatureStoreClientWrapper.__update_availability, the ReadWriteLock may not be released if an exception occurs after acquisition. This happens in two places: when updating __last_available and when starting self.__poller, which could lead to a deadlock.
Additional Locations (1)
| def status(self) -> DataStoreStatus: | ||
| self.__lock.rlock() | ||
| status = copy(self.__status) | ||
| self.__lock.runlock() |
There was a problem hiding this comment.
Bug: Locking Issue in DataStore Status Handling
The DataStoreStatusProviderImpl.status property's rlock/runlock pattern is not exception-safe. If copy(self.__status) raises an exception, the read lock remains held, potentially causing a deadlock.
Note
Adds FDv2 persistent store integration (read-only/read-write), data store status monitoring, and comprehensive tests.
FDv2 & Store:
FeatureStorewithStore.with_persistence(...); respectDataStoreMode.READ_ONLYvsREAD_WRITEfor writes.FeatureStoreClientWrapperto sortinitdata and emitDataStoreStatusavailability updates with recovery polling.commit(); replace prints withlog.Configuration:
DataSystemConfigwithdata_store_modeand optionaldata_store; makeprimary_synchronizeroptional.ConfigBuilder.data_store(...); providedaemon(...)(read-only) andpersistent_store(...)(read-write) helpers.DataStoreModeenum.Status APIs:
DataStoreStatusProviderImplwith listener support and monitoring detection.DataStoreStatuswith equality semantics.Tests:
test_fdv2_persistence.pycovering read-only/write modes, delta and delete persistence, and status provider/monitoring.Written by Cursor Bugbot for commit e88a705. This will update automatically on new commits. Configure here.