fix bug where an edge case exception causes a query to not be removed… #88
Conversation
… from active running queries
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce a Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sidecar/app/query/base.py (1)
204-208: Approve changes with suggestions for improvementThe introduction of the try-finally block effectively addresses the PR objective by ensuring that queries are always removed from
running_queries, even in edge cases where exceptions occur. This change improves the reliability of resource management and fixes the reported bug.However, consider the following suggestions to further enhance error handling and maintain system observability:
- Add explicit exception handling to log the error before it's potentially suppressed.
- Re-raise the exception after cleanup to inform the caller about the failure.
Here's a suggested implementation that incorporates these improvements:
def run_query(self, query: Query): if not self.capacity_available: raise MaxQueriesRunningError( f"Only {self.max_parallel_queries} allowed. Currently running {self}" ) self.running_queries[query.query_id] = query try: query.start() except Exception as e: query.logger.exception(f"Error occurred while running query: {e}") raise # Re-raise the exception after logging finally: # always remove this del self.running_queries[query.query_id]This implementation ensures that:
- Exceptions are logged, maintaining visibility into errors.
- The query is always removed from
running_queries.- The original exception is re-raised, allowing callers to handle or respond to the error appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
sidecar/tests/app/query/test_base.py (2)
124-124: Fix typo in function nameThere's a typo in the function name. "manger" should be "manager".
-def test_query_manger_run_query_exception(): +def test_query_manager_run_query_exception():
124-140: Add a comment explaining the test's purposeConsider adding a docstring or comment to explain the purpose of this test. This will help other developers understand the specific scenario being tested.
Add a comment like this at the beginning of the function:
def test_query_manager_run_query_exception(): """ Test that when an exception occurs during query execution: 1. The exception is properly raised 2. The query is removed from running_queries 3. The query manager's capacity is restored """🧰 Tools
🪛 Ruff
134-134:
pytest.raises(Exception)should be considered evil(B017)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- sidecar/tests/app/query/test_base.py (1 hunks)
🧰 Additional context used
🪛 Ruff
sidecar/tests/app/query/test_base.py
134-134:
pytest.raises(Exception)should be considered evil(B017)
🔇 Additional comments (1)
sidecar/tests/app/query/test_base.py (1)
124-140: LGTM: Test logic is soundThe overall logic of the test is good. It correctly verifies that:
- The exception is raised
- The
startmethod is called exactly once- The query is not left in the
running_querieslist- The query manager's capacity is available after the exception
These checks ensure that the
QueryManagerhandles exceptions gracefully and maintains its state correctly.🧰 Tools
🪛 Ruff
134-134:
pytest.raises(Exception)should be considered evil(B017)
Co-authored-by: Alex Koshelev <akoshelev@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sidecar/tests/app/query/test_base.py (1)
124-140: Improve exception handling specificityThe test function effectively verifies the behavior of
QueryManagerwhen an exception occurs during query execution. However, using a broadExceptionclass makes the test less precise and could potentially catch unintended exceptions.Consider implementing the following improvements:
- Define a specific exception class for query start errors:
class QueryStartError(Exception): pass
- Update the test to use this specific exception:
def mock_exception(): - raise Exception + raise QueryStartError("Error starting query") with mock.patch( "sidecar.app.query.base.Query.start", side_effect=mock_exception ) as mock_start: - with pytest.raises(Exception): + with pytest.raises(QueryStartError): query_manager.run_query(query)These changes will make the test more precise and address the static analysis warning about using
pytest.raises(Exception).🧰 Tools
🪛 Ruff
134-134:
pytest.raises(Exception)should be considered evil(B017)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- sidecar/tests/app/query/test_base.py (1 hunks)
🧰 Additional context used
🪛 Ruff
sidecar/tests/app/query/test_base.py
134-134:
pytest.raises(Exception)should be considered evil(B017)
… from active running queries
Summary by CodeRabbit