Enhance deletion for set of resources in parallel#2107
Enhance deletion for set of resources in parallel#2107cb-github-robot merged 1 commit intocloud-barista:mainfrom
Conversation
Signed-off-by: Seokho Son <shsongist@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the deletion of resources by implementing parallel processing with CSP-specific concurrency limits and improved error handling. The main goal is to optimize performance when deleting multiple resources by grouping them by cloud service provider (CSP) connection and processing them concurrently.
- Implements parallel deletion with connection-based grouping and semaphore-controlled concurrency
- Adds comprehensive logging for debugging and monitoring deletion operations
- Fixes JSON serialization issues by avoiding mutex in response structures
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/interface/rest/server/resource/common.go | Adds detailed logging and creates clean response structure without mutex to avoid JSON serialization issues |
| src/core/resource/common.go | Implements parallel deletion logic with CSP connection grouping, semaphore-based concurrency control, and enhanced error handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func getResourceConnectionName(nsId, resourceType, resourceId string) (string, error) { | ||
| // For performance, try to extract connection name from resourceId pattern first | ||
| // Many resources follow the pattern: {connectionName}-{resourceName} | ||
| parts := strings.Split(resourceId, "-") |
There was a problem hiding this comment.
The function assumes resource IDs follow a specific naming pattern (connectionName-resourceName) but this assumption may not hold for all resources. Consider adding validation or documentation about this assumption.
| errChan <- err // Send error to the error channel | ||
| } | ||
| // Create semaphores for each connection (limit concurrent operations per CSP) | ||
| const maxConcurrentPerCSP = 20 |
There was a problem hiding this comment.
The hardcoded concurrency limit of 20 should be configurable or at least documented with justification for this specific value. Different CSPs may have different rate limits.
| const maxConcurrentPerCSP = 20 | |
| maxConcurrentPerCSP := getMaxConcurrentPerCSP() // configurable via env var CBTB_MAX_CONCURRENT_PER_CSP, default 20 |
| log.Debug().Msgf("Starting deletion of %s:%s (connection: %s)", resourceType, resourceId, connName) | ||
|
|
||
| // Minimal random sleep to avoid thundering herd (reduced significantly) | ||
| common.RandomSleep(0, 100) |
There was a problem hiding this comment.
The magic number 100 for random sleep duration should be defined as a named constant or made configurable, with documentation explaining its purpose.
| common.RandomSleep(0, 100) | |
| common.RandomSleep(0, maxRandomSleepMsForDeletion) |
| select { | ||
| case errChan <- err: | ||
| // Successfully sent error to channel | ||
| case <-time.After(10 * time.Millisecond): |
There was a problem hiding this comment.
The timeout value of 10 milliseconds for error channel operations should be defined as a named constant with documentation explaining the rationale for this specific duration.
| case <-time.After(10 * time.Millisecond): | |
| case <-time.After(errorChanTimeout): |
| errString = " (" + err.Error() + ")" | ||
|
|
||
| // Safe error channel send - check if channel is still open | ||
| if atomic.LoadInt32(&errChanClosed) == 0 { |
There was a problem hiding this comment.
The atomic flag check for channel closure has a race condition. Between checking the flag and sending to the channel, another goroutine could close the channel, potentially causing a panic. Consider using a more robust pattern like a context for cancellation.
| // Safely close the error channel with atomic flag | ||
| if atomic.CompareAndSwapInt32(&errChanClosed, 0, 1) { | ||
| close(errChan) | ||
| } |
There was a problem hiding this comment.
This atomic operation for channel closure protection is complex and error-prone. Consider using sync.Once or a context-based approach for cleaner concurrency control.
| } | |
| // Safely close the error channel using sync.Once | |
| errChanCloseOnce.Do(func() { close(errChan) }) |
|
/approve |
No description provided.