CU-86bzu8m1g - Increase Lease in Dgraph while restoring Backup using API #326
Conversation
WalkthroughThe changes introduce two new functions in the Dgraph restore API: Changes
Possibly related PRs
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 (2)
devops/linux/docker/backup_restore/dgraph-restore-api.py (2)
15-31: LGTM with a minor suggestionThe
get_current_leasefunction is well-implemented with proper error handling and clear output. Good job on usingrequests.raise_for_status()to handle HTTP errors.A minor suggestion for improvement:
Consider using a logging library instead of print statements for better control over output in different environments.Replace print statements with logging:
+import logging + +logging.basicConfig(level=logging.INFO) +logger = logging.getLogger(__name__) def get_current_lease(endpoint): """Fetches the current maxLeasedUid from the Dgraph Zero /state endpoint.""" try: response = requests.get(f"{endpoint}/state") response.raise_for_status() # Raise an exception for HTTP errors data = response.json() # Extracting the maxLeasedUid from the JSON response max_leased_uid = data.get('maxUID', None) if max_leased_uid is not None: - print(f"Current maxLeasedUid: {max_leased_uid}") + logger.info(f"Current maxLeasedUid: {max_leased_uid}") return max_leased_uid else: - print("Error: maxLeasedUid not found in the response.") + logger.error("maxLeasedUid not found in the response.") return None except requests.RequestException as e: - print(f"Error fetching current lease: {e}") + logger.error(f"Error fetching current lease: {e}") return None
Line range hint
1-116: Summary of review for dgraph-restore-api.pyThe changes introduced in this file align well with the PR objective of increasing the lease in Dgraph while restoring backup using API. The new functions
get_current_leaseandincrease_leaseprovide the necessary functionality to manage leases effectively.Key points for improvement:
- Consider using a logging library instead of print statements for better output control.
- In the
increase_leasefunction:
- Use POST instead of GET for the /assign endpoint to adhere to RESTful principles.
- Implement more specific exception handling.
- Avoid logging potentially sensitive response content.
- Add error handling for the
increase_leasecall in theprocess_json_datafunction.- Make the
new_lease_valueconfigurable through environment variables for flexibility.These improvements will enhance the robustness, security, and maintainability of the code. Overall, the changes are on the right track and with these refinements, the implementation will be solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- devops/linux/docker/backup_restore/dgraph-restore-api.py (3 hunks)
🔇 Additional comments (3)
devops/linux/docker/backup_restore/dgraph-restore-api.py (3)
33-52:⚠️ Potential issueImprove HTTP method, error handling, and security in
increase_leasefunctionWhile the overall logic of the
increase_leasefunction is sound, there are several areas for improvement:
HTTP Method: Using a GET request for an operation that modifies server state (increasing the lease) is not RESTful. Consider using a POST or PUT request instead.
Error Handling: The current error handling is too broad. Consider catching more specific exceptions and handling them accordingly.
Security: Printing the entire response content on error could potentially leak sensitive information. It's better to log a sanitized version of the error.
Type Conversion: The type conversion of
current_valueto int is done after the None check, which could lead to a TypeError ifget_current_leasereturns None.Here's a suggested refactor addressing these points:
def increase_lease(endpoint, new_value): """Increases the lease value if it's below the new_value.""" current_value = get_current_lease(endpoint) if current_value is None: logger.error("Unable to fetch the current lease value. Exiting.") return try: current_value = int(current_value) except ValueError: logger.error(f"Invalid current lease value: {current_value}") return if current_value < new_value: try: data = {'what': 'uids', 'num': new_value} response = requests.post(f"{endpoint}/assign", json=data) response.raise_for_status() logger.info(f"Lease increased to {new_value}.") except requests.exceptions.HTTPError as e: logger.error(f"HTTP error occurred while increasing lease: {e}") except requests.exceptions.RequestException as e: logger.error(f"Error occurred while increasing lease: {e}") else: logger.info(f"Current lease ({current_value}) is already greater than or equal to {new_value}. No action taken.")This refactored version:
- Uses POST instead of GET for the /assign endpoint.
- Handles type conversion of
current_valuemore safely.- Uses more specific exception handling.
- Avoids logging potentially sensitive response content.
- Uses logging instead of print statements for consistency with earlier suggestions.
#!/bin/bash # Check if the /assign endpoint is used with POST method elsewhere in the codebase rg --type python "requests\.post.*?/assign" -g '!dgraph-restore-api.py'
105-105:⚠️ Potential issueAdd error handling for
increase_leasecallThe addition of
increase_lease(endpoint, new_lease_value)at the beginning ofprocess_json_datais a good approach to ensure sufficient lease before processing. However, there's currently no error handling if this operation fails.Consider adding error handling to ensure the process doesn't continue if the lease increase fails. Here's a suggested modification:
def process_json_data(golden_records): if not increase_lease(endpoint, new_lease_value): logger.error("Failed to increase lease. Aborting process.") return for golden_record in golden_records: # ... rest of the function ...This change assumes that you modify the
increase_leasefunction to return a boolean indicating success or failure. If you prefer not to modifyincrease_lease, you could wrap the call in a try-except block instead.#!/bin/bash # Check if increase_lease is used elsewhere in the codebase and if it's consistently handled rg --type python "increase_lease\(.*?\)" -g '!dgraph-restore-api.py'
12-14: Verify endpoint change and considernew_lease_valueimplications
The endpoint has been changed to use port 6080. Please confirm if this is the correct port for communicating with the Dgraph Zero node.
The
new_lease_valueis set to a very large number (50 trillion). While this may be necessary for your use case, consider the following:
- Ensure this value aligns with your system's capacity and expected data growth.
- Document the reasoning behind this specific value for future reference.
- Consider making this value configurable (e.g., through environment variables) for easier adjustments in different environments.
…estoring-Backup-using-API
…estoring-Backup-using-API
MatthewErispe
deleted the
CU-86bzu8m1g_Increase-Lease-in-Dgraph-while-restoring-Backup-using-API
branch
Summary by CodeRabbit