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: disabling merge before processor.Store(...) - default true #5338

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Sidddddarth
Copy link
Member

@Sidddddarth Sidddddarth commented Dec 4, 2024

Description

Disable merging subJobs before storing them in processor.
In cases where there's heavy multiplexing, it could be better to save each subJob's transformed jobs.
Also helpful in cases where transformation is the limiting step in processor pipeline.
Router sees a continuous input of jobs instead of once every few seconds.
Put behind a flag(default allows writing each subJob right away).

Linear Ticket

Resolves PIPE-1785

Security

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

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.73%. Comparing base (7ffac25) to head (274bf8a).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5338      +/-   ##
==========================================
- Coverage   74.78%   74.73%   -0.05%     
==========================================
  Files         433      437       +4     
  Lines       60979    61236     +257     
==========================================
+ Hits        45601    45766     +165     
- Misses      12848    12928      +80     
- Partials     2530     2542      +12     

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

@Sidddddarth Sidddddarth requested a review from lvrach December 6, 2024 07:19
@Sidddddarth Sidddddarth marked this pull request as ready for review December 9, 2024 05:11
Comment on lines +108 to 117
if w.handle.config().disableStoreMerge.Load() {
if firstSubJob {
w.handle.Store(w.partition, subJob)
continue
}
mergedJob.merge(subJob)
w.handle.Store(w.partition, mergedJob)
firstSubJob = true
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Sidddddarth I'm a bit confused by this. Shouldn't feature flags be backwards compatible? Meaning, given the default of the new setting, nothing should change in terms of behaviour.

I see that the default for the new setting is true. In that case we get into the new if on line 108 but we're not doing what we have on main. The logic is different now.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right
I wanted to make this the new behaviour. But it might not make sense in all cases
Will change the default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants