check to make sure helpers need to be killed before sending kill signal #89
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes involve modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- sidecar/app/query/ipa.py (2 hunks)
- sidecar/app/query/status.py (1 hunks)
- sidecar/tests/app/query/test_status.py (1 hunks)
🔇 Additional comments (4)
sidecar/app/query/status.py (2)
23-30: Consider broader implications of the newfrom_jsonmethodThe addition of the
from_jsonmethod to theStatusenum is a good step towards standardizing status parsing across the project. This aligns with the PR objective of improving status checking logic.While this change doesn't directly address the issue of checking if helpers need to be killed before sending a kill signal, it could be a crucial building block for implementing such checks. By providing a standardized way to parse status from JSON, it enables more robust status checking in other parts of the code where the kill signal logic is implemented.
Consider the following to maximize the impact of this change:
- Update any existing code that parses status from JSON to use this new method, ensuring consistency across the codebase.
- Review the areas of code responsible for sending kill signals, and consider how this new method can be utilized to improve the logic there.
- Update documentation to reflect this new way of parsing status, ensuring that other developers are aware of and use this method.
To help identify areas where this new method could be utilized, we can search for places in the code that are currently parsing status manually:
#!/bin/bash # Search for potential manual status parsing rg --type python 'status.*=.*get\("status"'This will help identify areas of the code that might benefit from using the new
from_jsonmethod.
23-30: 🛠️ Refactor suggestionImprove error handling and input validation in
from_jsonmethodThe new
from_jsonmethod is a good addition for parsing status from JSON input. However, there are a few areas where it could be improved:
- Input validation: The method doesn't handle cases where the input is not a dictionary or is None.
- Silent error handling: Returning
UNKNOWNfor any invalid status might mask errors.- Type checking: The method doesn't validate that the "status" value is a string before using it.
Consider refactoring the method to address these issues:
@classmethod def from_json(cls, response: Optional[dict[str, Any]]) -> Status: if not isinstance(response, dict): return cls.UNKNOWN status_str = response.get("status") if not isinstance(status_str, str): return cls.UNKNOWN try: return cls[status_str.upper()] except KeyError: return cls.UNKNOWNThis refactored version:
- Handles cases where the input is not a dictionary.
- Checks if the "status" value is a string.
- Converts the status string to uppercase for case-insensitive matching.
Additionally, consider logging warnings when returning
UNKNOWNdue to invalid input, to help with debugging.To ensure this method is used correctly throughout the codebase, we can search for its usage:
This will help verify that the method is being called with the correct input type across the project.
sidecar/tests/app/query/test_status.py (1)
118-131: New test function aligns with PR objectivesThe addition of the
test_status_from_json()function is a positive change that aligns with the PR objectives. It provides test coverage for the newfrom_jsonmethod of theStatusclass, which is likely part of the enhanced status checking logic mentioned in the PR summary.The test function covers the main scenarios for parsing status from JSON input, including valid statuses, invalid inputs, and empty JSON. This will help ensure the robustness of the status checking mechanism, which is crucial for addressing the issue of unnecessary kill signals being sent.
While the current implementation is good, the suggested improvements in the previous comment would further enhance the test coverage and maintainability of the code.
sidecar/app/query/ipa.py (1)
275-279: Ensure Python 3.10 compatibility for 'match' statementsThe use of
matchandcasesyntax requires Python 3.10 or higher. Verify that the runtime environment supports Python 3.10. If compatibility with earlier Python versions is necessary, refactor thematchstatements to useif-elifconditions.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
sidecar/app/settings.py (1)
65-67: LGTM! Consider a minor optimization.The new
other_helpersproperty is well-implemented and provides a useful way to access helpers with different roles. The implementation is concise and efficient.For a slight performance improvement, especially if
self._helpersis large, consider using a generator expression instead of a list comprehension:@property def other_helpers(self) -> list[Helper]: return list(helper for helper in self._helpers.values() if helper.role != self.role)This change would create the list only when it's needed, potentially saving memory if the property is accessed but the result is not fully iterated.
sidecar/app/query/ipa.py (3)
31-33: Approve changes with a suggestion for error handlingThe simplification of the
send_kill_signalsmethod is a good improvement. It reduces the complexity of the code and aligns with the PR objective of preventing unnecessary kill signals.Consider adding error handling to catch potential exceptions from the
kill_querymethod:for helper in settings.other_helpers: - response = helper.kill_query(self.query_id) - self.logger.info(response) + try: + response = helper.kill_query(self.query_id) + self.logger.info(response) + except Exception as e: + self.logger.error(f"Failed to kill query on helper: {e}")This will ensure that if one helper fails, it doesn't prevent sending kill signals to the remaining helpers.
240-264: Approve changes with a suggestion for timeout mechanismThe updates to the
runmethod inIPACoordinatorWaitForHelpersStepare excellent improvements. The use ofget_current_query_statusand the match statement with Status enum values makes the code more robust and readable.Consider using a more pythonic approach for the timeout mechanism:
- max_unknonwn_status_wait_time = 100 - current_unknown_status_wait_time = 0 - loop_wait_time = 1 + max_unknown_status_wait_time = 100 + start_time = time.time() while True: status = helper.get_current_query_status(self.query_id) match status: case Status.IN_PROGRESS: break case Status.KILLED | Status.NOT_FOUND | Status.CRASHED: self.success = False return case Status.STARTING | Status.COMPILING | Status.WAITING_TO_START: # keep waiting while it's in a startup state continue case Status.UNKNOWN | Status.NOT_FOUND: # eventually fail if the status is unknown or not found # for ~100 seconds - current_unknown_status_wait_time += loop_wait_time - if ( - current_unknown_status_wait_time - >= max_unknonwn_status_wait_time - ): + if time.time() - start_time >= max_unknown_status_wait_time: self.success = False return - time.sleep(1) + time.sleep(1)This approach is more accurate and easier to understand, as it uses the actual elapsed time rather than an approximation based on loop iterations.
343-348: Approve changes with a suggestion for error handlingThe renaming of
send_terminate_signalstosend_finish_signalsand the simplification of the method are good improvements. They align with the naming convention used elsewhere and reduce the complexity of the code.Consider adding error handling to catch potential exceptions from the
finish_querymethod, similar to the suggestion forsend_kill_signals:def send_finish_signals(self): self.logger.info("sending finish signals") settings = get_settings() for helper in settings.other_helpers: - resp = helper.finish_query(self.query_id) - self.logger.info(resp) + try: + resp = helper.finish_query(self.query_id) + self.logger.info(resp) + except Exception as e: + self.logger.error(f"Failed to finish query on helper: {e}")This will ensure that if one helper fails, it doesn't prevent sending finish signals to the remaining helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- pyproject.toml (1 hunks)
- sidecar/app/helpers.py (2 hunks)
- sidecar/app/query/ipa.py (3 hunks)
- sidecar/app/settings.py (1 hunks)
- sidecar/tests/app/query/test_status.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sidecar/tests/app/query/test_status.py
🔇 Additional comments (3)
sidecar/app/settings.py (1)
65-67: Good integration with existing code.The new
other_helpersproperty integrates well with the existing codebase:
- It follows the established naming conventions and coding style.
- It complements the existing
helpersproperty, providing a convenient way to access a subset of helpers.- The addition doesn't introduce any side effects or modify existing behavior.
To further improve the code, consider adding a docstring to explain the purpose and return value of this property. For example:
@property def other_helpers(self) -> list[Helper]: """ Returns a list of Helper instances excluding the one with the same role as the current instance. This can be useful for inter-helper communication or coordination. Returns: list[Helper]: A list of Helper instances with different roles. """ return [helper for helper in self._helpers.values() if helper.role != self.role]sidecar/app/query/ipa.py (1)
Line range hint
1-424: Overall assessment: Improvements align with PR objectivesThe changes made to
sidecar/app/query/ipa.pyare well-aligned with the PR objectives. They simplify the code, improve error handling, and prevent unnecessary kill signals. The use of helper methods likekill_queryandfinish_queryinstead of direct HTTP requests reduces complexity and potential errors in URL construction.The updated
runmethod inIPACoordinatorWaitForHelpersStepnow handles different statuses more robustly and includes a timeout mechanism for unknown statuses, which prevents potential infinite loops.These improvements contribute to a more maintainable and reliable codebase. The suggested error handling additions will further enhance the robustness of the system.
sidecar/app/helpers.py (1)
1-1: Verifytomllibavailability for Python versions below 3.11The
tomllibmodule is available in Python 3.11 and above. If your project supports Python versions earlier than 3.11, consider using the third-partytomlilibrary for TOML parsing to maintain compatibility.To confirm the project's Python version compatibility, run the following script:
| current_unknown_status_wait_time | ||
| >= max_unknonwn_status_wait_time | ||
| ): | ||
| self.success = False |
There was a problem hiding this comment.
would that inform the UI that we failed to get a "good" status from helper and this is the reason why we couldn't start a query?
There was a problem hiding this comment.
when a Step finishes with self.success = False, it causes the Query to crash. The UI will show the report collector as crashed, and in the logs we'll see that the IPACoordinatorWaitForHelpersStep failed.
the failure we we're seeing was an issue with this kill signal failing with an http error. we wrap that in a try/except in #88 to make sure it always cleans up, but this change makes sure we don't send extra kill signals that don't need to be sent. I'm not entirely convinced this would fix the issue without #88, but it seems like it could be the issue.
here are the logs from Helper 1 in the crash:
associated with this error on the report collector:
Summary by CodeRabbit
New Features
Statusclass to handle JSON input for status retrieval.Helperclass for managing query statuses, including methods to construct URLs and manage query actions.Bug Fixes
UNKNOWNwhen the status key is missing.Tests
from_jsonmethod to ensure correct behavior with various JSON inputs.Chores
tomllibas part of the extra standard libraries.