feat: execute successfully completed actions once #206
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cirisked
reviewed
Jan 8, 2026
internal/controller/controller.go
Outdated
|
|
||
| startedActionsWg sync.WaitGroup | ||
| startedActions map[string]struct{} | ||
| completedActions map[string]time.Time |
There was a problem hiding this comment.
guess it's time to create a map + RWMutex as a separate struct
cirisked
reviewed
Jan 8, 2026
internal/controller/controller.go
Outdated
| now := time.Now() | ||
|
|
||
| s.startedActionsMu.Lock() | ||
| defer s.startedActionsMu.Unlock() |
There was a problem hiding this comment.
why are u locking on startedActionsMu here? i think it's better to lock on separate mutex
once again, i think we need simple concurrent map as separate struct
cirisked
reviewed
Jan 8, 2026
| observedMax := maxExecutingObserved | ||
| counts := make(map[string]int) | ||
| for k, v := range executionCounts { | ||
| counts[k] = v |
There was a problem hiding this comment.
btw, this is frustrating
i don't think linter should be that strict
cirisked
reviewed
Jan 8, 2026
cirisked
left a comment
cirisked
left a comment
There was a problem hiding this comment.
all good, but i'd like to have separate struct to handle concurrent map read/write ops
Tsonov
reviewed
Jan 8, 2026
internal/controller/controller.go
Outdated
| healthCheck *health.HealthzProvider | ||
| actionsMu sync.Mutex | ||
| startedActions map[string]struct{} // protected by actionsMu | ||
| completedActions map[string]int8 // protected by actionsMu |
Contributor
There was a problem hiding this comment.
[nit] I'd call it recentlyCompletedActions to be more accurrate
Tsonov
reviewed
Jan 8, 2026
Tsonov
reviewed
Jan 8, 2026
Tsonov
reviewed
Jan 8, 2026
Tsonov
approved these changes
Jan 9, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
actions might be executed at least twice as PollActions and AckAction are asynchronous operations and can sometimes race. Introduce a store for tracking recently completed actions to avoid that. The completed actions will be garbage collected after two polls when they are completed and successfully acked.