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: support generic rules to have routers drain events #3856

Merged
merged 11 commits into from
Nov 3, 2023

Conversation

Sidddddarth
Copy link
Member

@Sidddddarth Sidddddarth commented Sep 12, 2023

Description

The topic of draining events from a sync gave birth to this.
This is a way to pass generic rules by which (batch)router and processor(only jobRunID now) can drain events. How this is set to config is another story.

Routers, BatchRouters now have a drainer which provides the method:
Drain(job *jobsdb.JobT) (bool, string) to check if their jobs can be drained. Support for draining via configurable destinationIDs("Router.toAbortDestinationIDs") and jobRunID("RSources.toAbortJobRunIDs") present now. Adding more drain configurations should be straightforward -> just need to add logic in type drainer and (d *drainer) Drain(...).

Processor has a simpler approach, a new config field is added to accommodate configurations to drain by, jobRunId for now. Also injecting *config.Config into processor now. Plan is to proliferate it's use instead of config.Default in processor package(story for another PR).

Linear Ticket

drain events for a cancelled sync

Security

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

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (408046d) 71.75% compared to head (edf6dc1) 71.78%.
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3856      +/-   ##
==========================================
+ Coverage   71.75%   71.78%   +0.03%     
==========================================
  Files         374      372       -2     
  Lines       54919    54987      +68     
==========================================
+ Hits        39407    39474      +67     
- Misses      13188    13190       +2     
+ Partials     2324     2323       -1     
Files Coverage Δ
processor/manager.go 92.68% <100.00%> (+0.68%) ⬆️
router/batchrouter/factory.go 100.00% <100.00%> (ø)
router/batchrouter/handle.go 72.05% <100.00%> (ø)
router/batchrouter/handle_async.go 8.62% <100.00%> (ø)
router/batchrouter/handle_lifecycle.go 62.00% <100.00%> (+0.77%) ⬆️
router/batchrouter/worker.go 88.72% <100.00%> (-0.06%) ⬇️
router/handle.go 70.75% <100.00%> (+0.86%) ⬆️
router/handle_lifecycle.go 95.45% <100.00%> (+0.10%) ⬆️
router/worker.go 81.72% <100.00%> (-0.40%) ⬇️
processor/processor.go 89.06% <95.31%> (+0.11%) ⬆️
... and 1 more

... and 13 files with indirect coverage changes

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

@Sidddddarth Sidddddarth force-pushed the chore.routerutils_GenericToBeDrained branch from c0c30de to 1b80237 Compare September 15, 2023 12:43
@Sidddddarth Sidddddarth requested review from cisse21 and chandumlg and removed request for cisse21 and chandumlg September 20, 2023 16:17
@Sidddddarth Sidddddarth force-pushed the chore.routerutils_GenericToBeDrained branch 2 times, most recently from f41a208 to 8591766 Compare September 25, 2023 12:56
@Sidddddarth Sidddddarth force-pushed the chore.routerutils_GenericToBeDrained branch from 8591766 to 90fdddf Compare October 6, 2023 07:32
@Sidddddarth Sidddddarth force-pushed the chore.routerutils_GenericToBeDrained branch from 2bcf64c to f257dd2 Compare October 9, 2023 13:15
RecordID interface{} `json:"record_id"`
MessageID string `json:"message_id"`
WorkspaceID string `json:"workspaceId"`
RudderAccountID string `json:"rudderAccountId"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Router has more parameters than batchrouter. Who is using rudderAccountId?

RecordID                interface{} `json:"record_id"`
WorkspaceID             string      `json:"workspaceId"`
RudderAccountID         string      `json:"rudderAccountId"`

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah
the new struct contains a combo of both

Copy link
Member Author

Choose a reason for hiding this comment

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

as for RudderAccountId, it is present in multiple structs in the repo.
I will tackle them in another PR. Or perhaps defer to integrations, since they seem to have introduced it.

Copy link
Member

Choose a reason for hiding this comment

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

RudderAccountID is used for OAuth destination IIRC

Copy link
Contributor

Choose a reason for hiding this comment

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

Nobody is reading it in server's code though, @sanpj2292 ?

router/utils/utils.go Outdated Show resolved Hide resolved
processor/processor.go Outdated Show resolved Hide resolved
processor/processor.go Outdated Show resolved Hide resolved
processor/processor.go Outdated Show resolved Hide resolved
router/batchrouter/handle_lifecycle.go Outdated Show resolved Hide resolved
router/utils/utils.go Outdated Show resolved Hide resolved
router/utils/utils.go Outdated Show resolved Hide resolved
router/batchrouter/handle_lifecycle.go Outdated Show resolved Hide resolved
router/utils/utils.go Outdated Show resolved Hide resolved
@Sidddddarth Sidddddarth force-pushed the chore.routerutils_GenericToBeDrained branch 2 times, most recently from a4fd722 to 1ecff8a Compare October 11, 2023 13:26
@Sidddddarth Sidddddarth force-pushed the chore.routerutils_GenericToBeDrained branch from 00e566b to c94e405 Compare October 31, 2023 08:37
@Sidddddarth Sidddddarth force-pushed the chore.routerutils_GenericToBeDrained branch 6 times, most recently from a897b87 to e6f2a40 Compare November 3, 2023 08:25
@Sidddddarth Sidddddarth force-pushed the chore.routerutils_GenericToBeDrained branch from e6f2a40 to edf6dc1 Compare November 3, 2023 11:06
@@ -1,6 +1,7 @@
package utils
Copy link
Member

Choose a reason for hiding this comment

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

the package name is not very indicative of its contents, and in general, util/misc names should be avoided.

We can have a type package for JobParameters.
and introduce a new drainer package for NewDrainer

@Sidddddarth Sidddddarth merged commit 6583364 into master Nov 3, 2023
37 checks passed
@Sidddddarth Sidddddarth deleted the chore.routerutils_GenericToBeDrained branch November 3, 2023 15:48
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.

4 participants