Hold back SQLLite commit for up to 5 seconds #3127
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements commit throttling for the DatabaseManager SQLite operations to reduce disk I/O. The change ensures that database commits only occur once every 5 seconds when the queue is empty, rather than after every database write operation.
Key changes:
- Added commit throttling with 5-second interval to reduce unnecessary disk writes
- Tracks last commit time to enforce throttling policy
- Includes comprehensive test coverage for throttling behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
apps/predbat/db_manager.py |
Implements commit throttling by tracking last_commit_time and only committing when queue is empty and 5+ seconds have elapsed |
apps/predbat/tests/test_db_manager.py |
Adds comprehensive test for commit throttling including initial commit, rapid writes, throttle expiry, and data persistence verification |
apps/predbat/unit_test.py |
Registers the new commit throttling test in the test registry |
| self.return_event = threading.Event() | ||
| self.api_started = False | ||
| self.last_success_timestamp = None | ||
| self.last_commit_time = datetime.now(timezone.utc) |
There was a problem hiding this comment.
The initialization of last_commit_time should be None instead of datetime.now(timezone.utc). The throttling logic at line 86 checks if last_commit_time is None to allow the first commit to happen immediately, but with this initialization, the first commit will be unnecessarily delayed by 5 seconds. This means new data written immediately after startup won't be committed until 5 seconds have passed.
Change line 30 to:
self.last_commit_time = NoneThis will ensure the first commit happens immediately when the queue is empty after the first write operation.
| self.last_commit_time = datetime.now(timezone.utc) | |
| self.last_commit_time = None |
| await asyncio.sleep(0.1) | ||
| await loop.run_in_executor(None, db_mgr.set_state_db, "sensor.test2", "20", {}) | ||
| await asyncio.sleep(0.1) | ||
| await loop.run_in_executor(None, db_mgr.set_state_db, "sensor.test3", "30", {}) | ||
| await asyncio.sleep(0.1) | ||
| await loop.run_in_executor(None, db_mgr.set_state_db, "sensor.test4", "40", {}) | ||
| await asyncio.sleep(0.1) |
There was a problem hiding this comment.
The sleep durations between writes may be insufficient to ensure proper queue processing in the test. The set_state_db method queues commands asynchronously via IPC and returns immediately without waiting for processing. The 0.1 second sleeps might not guarantee that each queued item is fully processed before checking last_commit_time.
Consider increasing the sleep duration to at least 0.2 seconds between writes (as used in line 549 for the first test), or add an explicit wait mechanism to ensure the queue is empty before proceeding to the next assertion. This would make the test more reliable and less prone to timing-related flakiness.
#3114