-
Notifications
You must be signed in to change notification settings - Fork 317
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
Conversation
Codecov ReportAttention:
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
☔ View full report in Codecov by Sentry. |
c0c30de
to
1b80237
Compare
f41a208
to
8591766
Compare
8591766
to
90fdddf
Compare
2bcf64c
to
f257dd2
Compare
RecordID interface{} `json:"record_id"` | ||
MessageID string `json:"message_id"` | ||
WorkspaceID string `json:"workspaceId"` | ||
RudderAccountID string `json:"rudderAccountId"` |
There was a problem hiding this comment.
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"`
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
a4fd722
to
1ecff8a
Compare
00e566b
to
c94e405
Compare
a897b87
to
e6f2a40
Compare
Co-authored-by: Aris Tzoumas <atzoumas@rudderstack.com>
e6f2a40
to
edf6dc1
Compare
@@ -1,6 +1,7 @@ | |||
package utils |
There was a problem hiding this comment.
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
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 intype 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 ofconfig.Default
in processor package(story for another PR).Linear Ticket
drain events for a cancelled sync
Security