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

chore(batchrouter): honour upload frequency when limitsReached if destination is failing #3874

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

atzoum
Copy link
Contributor

@atzoum atzoum commented Sep 15, 2023

Description

When a destination is failing there is no point in keep retrying jobs continuously even if there are a lot of jobs in the queue (limitsReached in jobsdb query).

Linear Ticket

PIPE-283

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@@ -245,7 +248,8 @@ func (brt *Handle) getWorkerJobs(partition string) (workerJobs []*DestinationJob
if batchDest, ok := destinationsMap[destID]; ok {
var processJobs bool
brt.lastExecTimesMu.Lock()
if limitsReached && !brt.forceHonorUploadFrequency { // if limits are reached, process all jobs regardless of their upload frequency
Copy link
Contributor Author

Choose a reason for hiding this comment

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

**Note: ** forceHonorUploadFrequency is being removed in #3871

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03% 🎉

Comparison is base (12f4d83) 68.81% compared to head (1a6bacb) 68.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3874      +/-   ##
==========================================
+ Coverage   68.81%   68.84%   +0.03%     
==========================================
  Files         351      351              
  Lines       52746    52752       +6     
==========================================
+ Hits        36299    36319      +20     
+ Misses      14130    14112      -18     
- Partials     2317     2321       +4     
Files Changed Coverage Δ
router/batchrouter/handle.go 61.15% <100.00%> (+0.31%) ⬆️
router/batchrouter/handle_lifecycle.go 61.74% <100.00%> (+0.12%) ⬆️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -116,6 +116,9 @@ type Handle struct {
lastExecTimesMu sync.RWMutex
lastExecTimes map[string]time.Time

failingDestinationsMu sync.RWMutex
failingDestinations map[string]bool
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
failingDestinations map[string]bool
failingDestinations map[string]struct{}

Is it possible to use empty structs here, since currently it is being used as set only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we would need to delete from the set when the destination starts succeeding. Both are equally valid solutions in my head. I went with the one that requires the less amount of lines :)

@atzoum atzoum merged commit ae989cd into master Sep 15, 2023
35 checks passed
@atzoum atzoum deleted the chore.batchrtFailures branch September 15, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants