Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: correctly delete applications using Celery workers #5787

Merged
merged 4 commits into from
Jul 1, 2024

Conversation

chazzhou
Copy link
Contributor

Description

This PR implements a robust application deletion process using Celery workers. The change ensures that app deletion is handled asynchronously, improving system reliability and laying the groundwork for better user experience. It includes the deletion of all related data, including tag bindings.

However, more work is needed to implement polling or WebSocket for actual front-end status updates. This PR focuses on the backend implementation and prepares the system for future frontend enhancements.

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Manually triggered app deletion and verified in the database that all related data (including tag bindings) are properly deleted
  • Tested error scenarios by intentionally causing failures in the deletion process
  • Verified that the Celery task retry mechanism works as expected

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods
  • optional I have made corresponding changes to the documentation
  • optional I have added tests that prove my fix is effective or that my feature works
  • optional New and existing unit tests pass locally with my changes

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. 🐞 bug Something isn't working 💪 enhancement New feature or request labels Jun 30, 2024
@chazzhou
Copy link
Contributor Author

cc. @crazywoola

@laipz8200
Copy link
Collaborator

Thank you for your PR! Could you please open an issue first, providing a detailed description of the problem, and include the issue link in this PR? This will help us quickly understand and review your PR.

@crazywoola crazywoola requested a review from takatost July 1, 2024 01:03
@chazzhou chazzhou requested a review from laipz8200 July 1, 2024 04:40
Copy link
Collaborator

@laipz8200 laipz8200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have a few questions. Please check the comments I left in the code.

@laipz8200 laipz8200 self-assigned this Jul 1, 2024
Copy link
Collaborator

@laipz8200 laipz8200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thank you.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 1, 2024
@laipz8200 laipz8200 merged commit cb09dbe into langgenius:main Jul 1, 2024
5 checks passed
@chazzhou chazzhou deleted the asyc-app-deletion branch July 1, 2024 06:49
@takatost takatost mentioned this pull request Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 💪 enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants