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

[ptp4u] signaling messages done by workers #201

Merged
merged 1 commit into from Aug 21, 2022
Merged

Conversation

leoleovich
Copy link
Contributor

Summary

Currently server directly responds to the signaling messages (grants).
However, we already have powerful mechanism of sending messages (sync/fwup/announce/delayResp) via workers.
This change will leverage workers to send grants, completely removing send logic from the server.
This will unblock cancellation messages being generated within the subscription on cancel.

Test Plan

Tcpdump/Wireshard

Screen Shot 2022-08-20 at 18 44 50

Works in prod

Unittests

Lint issue is unrelated

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 20, 2022
@leoleovich
Copy link
Contributor Author

Lint issue is unrelated ^

@codecov-commenter
Copy link

Codecov Report

Merging #201 (d57cde4) into main (f5cf74e) will decrease coverage by 0.03%.
The diff coverage is 57.47%.

@@            Coverage Diff             @@
##             main     #201      +/-   ##
==========================================
- Coverage   60.89%   60.86%   -0.04%     
==========================================
  Files          86       86              
  Lines        7910     7916       +6     
==========================================
+ Hits         4817     4818       +1     
- Misses       2657     2663       +6     
+ Partials      436      435       -1     
Impacted Files Coverage Δ
ptp/ptp4u/server/config.go 85.71% <ø> (ø)
ptp/ptp4u/server/server.go 35.06% <40.00%> (-0.36%) ⬇️
ptp/ptp4u/server/worker.go 56.75% <56.41%> (-2.38%) ⬇️
ptp/ptp4u/server/subscription.go 98.52% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@vvfedorenko vvfedorenko left a comment

Choose a reason for hiding this comment

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

There are too many copy-paste lines of code for each message type in the worker go-routine. Could we factorise it? Or I can do it in separate PR

[ptp4l] only send 1 extra announce messages
@leoleovich
Copy link
Contributor Author

They are all kinda different. Some send via event socket and read TX timestamp, some send via general and don't. We could make it nicer I suppose. But not sure making this diff even bigger is good :)

@vvfedorenko
Copy link
Contributor

OK, NVM, I will clean it up later

@vvfedorenko vvfedorenko merged commit 8cdf214 into main Aug 21, 2022
@vvfedorenko vvfedorenko deleted the signaling_worker branch August 21, 2022 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants