-
Notifications
You must be signed in to change notification settings - Fork 322
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
feat: isolate server ut communication #5430
base: master
Are you sure you want to change the base?
Conversation
56c9769
to
f16e30e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5430 +/- ##
==========================================
- Coverage 74.79% 74.01% -0.78%
==========================================
Files 440 447 +7
Lines 61670 62406 +736
==========================================
+ Hits 46125 46191 +66
- Misses 13002 13673 +671
+ Partials 2543 2542 -1 ☔ View full report in Codecov by Sentry. |
b31ee2d
to
1771a8e
Compare
e471d37
to
abd8358
Compare
09ad557
to
4bf68a2
Compare
processor/internal/trackingplan_validation/trackingplan_validation.go
Outdated
Show resolved
Hide resolved
processor/internal/destination_transformer/destination_transformer.go
Outdated
Show resolved
Hide resolved
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.
-
Can I assume the implementation logic(in user_trans, dest_trans) is exactly same as copied from transformer.go? planning to skip reviewing that part for just skim it through.
-
router transformer will remain as is? it’s only transformation in processor that we’re splitting out rt?
-
Looks like we’ve not replaced function calls to use the new sendrequest function. do we plan to do that in follow up PR?
-
Are we okay to have
types.TransformerEvent
as param for all implementations(in ServiceClient -> SendRequest)? Will we lose flexibility to have same struct? or is it going to be same and we choose what to populate in that struct?
Yeah... I just got rid of a few things here and there which are relevant to individual components but yeah in this PR we can assume that it is going to be the same
Yes... it is only the processor transformer module we are splitting out
That is the only part remaining where I will put it under a flag so that we can switch between older and newer implementation
Didn't want to make too many changes in one PR but the idea is to abstract out things in this PR and then change the structs with the appropriate data over the subsequent PR's |
processor/internal/destination_transformer/destination_transformer.go
Outdated
Show resolved
Hide resolved
endlessBackoff.MaxInterval = t.config.maxRetryBackoffInterval.Load() | ||
|
||
// endless backoff loop, only nil error or panics inside | ||
_ = backoff.RetryNotify( |
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.
consider dropping endless backoff here too
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.
This is needed from what I discussed with @Jayachand
5c1c564
to
f01fe43
Compare
Description
< Replace with adequate description for this PR as per Pull Request document >
Linear Ticket
< Replace with Linear Link ( create or search linear ticket) or >
Security