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.

Sign up for GitHub

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

Jump to bottom

remote target and transmission protocol #13357

Merged
merged 2 commits into from
May 30, 2024
Merged

remote target and transmission protocol #13357

merged 2 commits into from
May 30, 2024

Conversation

ettec
Copy link
Collaborator

@ettec ettec commented May 29, 2024 •
edited
Loading

ks-119

@ettec ettec force-pushed the ks-119/remote-transmission-protocol branch from bb2906f to 591bacc Compare May 29, 2024 16:34
@ettec ettec marked this pull request as ready for review May 29, 2024 16:35
@ettec ettec requested a review from a team as a code owner May 29, 2024 16:35
@ettec ettec force-pushed the ks-119/remote-transmission-protocol branch from 591bacc to b49761e Compare May 29, 2024 16:35
krehermann
krehermann reviewed May 29, 2024
View reviewed changes
core/capabilities/remote/target/caller.go Outdated Show resolved Hide resolved
core/capabilities/remote/target/caller.go Outdated Show resolved Hide resolved
core/capabilities/remote/target/caller.go Outdated Show resolved Hide resolved
core/capabilities/remote/target/receiver.go Outdated Show resolved Hide resolved
core/capabilities/remote/target/request/caller_request.go Outdated Show resolved Hide resolved
core/capabilities/remote/target/request/caller_request.go Outdated Show resolved Hide resolved
core/capabilities/remote/target/request/caller_request.go Outdated Show resolved Hide resolved
core/capabilities/transmission/transmission.go Show resolved Hide resolved
bolekk
bolekk reviewed May 30, 2024
View reviewed changes
core/capabilities/remote/target/caller.go Outdated Show resolved Hide resolved
core/capabilities/remote/target/receiver.go Outdated
}

// A request is uniquely identified by the message id and the hash of the payload to prevent a malicious
// actor from sending a different payload with the same message id
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not a fan of this. I'm worried that it'll make debugging harder. IMO a more natural logic will be to cluster messages based on WorkflowExecutionID and then pick F+1 identical payloads once we have them. Your logic achieves the same outcome so I'm not going to block it.

core/capabilities/remote/target/request/receiver_request.go Outdated Show resolved Hide resolved
bolekk
bolekk previously approved these changes May 30, 2024
View reviewed changes
@ettec ettec force-pushed the ks-119/remote-transmission-protocol branch from 1605484 to 77271e0 Compare May 30, 2024 11:41
@ettec ettec force-pushed the ks-119/remote-transmission-protocol branch from 77271e0 to 5fcc3b3 Compare May 30, 2024 11:42
cedric-cordenier
cedric-cordenier reviewed May 30, 2024
View reviewed changes
core/capabilities/remote/target/caller.go Outdated
dispatcher types.Dispatcher
requestTimeout time.Duration

messageIDToCallerRequest map[string]*request.CallerRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you choose to put the Request objects in separate packages? In my mind organising all of the objects that talk to one another into the same package feels like it makes more sense (so caller + caller request; receiver + its request).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to prevent accidental call of non-threadsafe methods, just caution really

@ettec ettec force-pushed the ks-119/remote-transmission-protocol branch from 65a6b37 to 65bf532 Compare May 30, 2024 13:32
@ettec ettec force-pushed the ks-119/remote-transmission-protocol branch from 22676f7 to 20a3061 Compare May 30, 2024 13:51
@ettec ettec force-pushed the ks-119/remote-transmission-protocol branch from b8cde07 to 12962e4 Compare May 30, 2024 14:02
@bolekk bolekk enabled auto-merge May 30, 2024 16:10
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
5 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
87.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@bolekk bolekk added this pull request to the merge queue May 30, 2024
Merged via the queue into develop with commit 21f38e5 May 30, 2024
109 checks passed
@bolekk bolekk deleted the ks-119/remote-transmission-protocol branch May 30, 2024 16:46
@github-actions github-actions bot mentioned this pull request May 30, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krehermann krehermann krehermann left review comments

@cedric-cordenier cedric-cordenier cedric-cordenier approved these changes

@bolekk bolekk bolekk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ettec @bolekk @krehermann @cedric-cordenier