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

Make Candidate an interface #48

Merged
merged 2 commits into from
May 24, 2019
Merged

Make Candidate an interface #48

merged 2 commits into from
May 24, 2019

Conversation

Sean-Der
Copy link
Member

This change will allow us to have custom logic and members
per interface type. Relay candidates will have a completely different
read loop, and candidate specific state.

Relates to #47

This change will allow us to have custom logic and members
per interface type. Relay candidates will have a completely different
read loop, and candidate specific state.

Relates to #47
@Sean-Der
Copy link
Member Author

This diff is mostly me moving files around. I would say the big thing I would like to know is if people agree with how this is implemented.

Most candidates share all the same logic, but TURN needs to have its own readLoop and will have state not shared by other candidates.

@codecov-io
Copy link

codecov-io commented May 24, 2019

Codecov Report

Merging #48 into master will increase coverage by 0.43%.
The diff coverage is 89.85%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #48      +/-   ##
=========================================
+ Coverage   75.27%   75.7%   +0.43%     
=========================================
  Files          16      20       +4     
  Lines        1189    1202      +13     
=========================================
+ Hits          895     910      +15     
+ Misses        237     235       -2     
  Partials       57      57
Impacted Files Coverage Δ
selection.go 75.24% <100%> (ø) ⬆️
agent.go 79.33% <100%> (-0.17%) ⬇️
candidatepair.go 86.53% <100%> (ø) ⬆️
gather.go 34.53% <50%> (-0.22%) ⬇️
candidate_host.go 84.61% <84.61%> (ø)
candidate_relay.go 88.23% <88.23%> (ø)
candidate_peer_reflexive.go 88.23% <88.23%> (ø)
candidate_server_reflexive.go 88.23% <88.23%> (ø)
candidate_base.go 89.71% <89.71%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a060774...f2826a4. Read the comment docs.

candidate.go Outdated Show resolved Hide resolved
candidate.go Outdated Show resolved Hide resolved
@hugoArregui
Copy link
Member

I have no strong opinion about this change mainly because I don't know what's involved for the relay candidates. If the only change is the readLoop and the state maybe you could consider abstracting that out? (something like the thing we did for the agent selector)

Remove setLastSent and setLastReceived from Candidate interface
@Sean-Der
Copy link
Member Author

@hugoArregui thanks for the review (speaking of ICE reviews we need to land all your work!)

As long as you think the code is still pleasant to work with I would like to keep going with it. The relay change will involve

  • Custom read behavior
  • Custom write behavior
  • Custom behavior when creating new candidates (for each candidate of other type we need to create a new permission on our TURN server)

@Sean-Der Sean-Der requested a review from hugoArregui May 24, 2019 22:00
@Sean-Der Sean-Der merged commit 14ec800 into master May 24, 2019
@Sean-Der Sean-Der deleted the issue-47 branch May 24, 2019 22:56
@enobufs
Copy link
Member

enobufs commented May 25, 2019

Later might be better than never... I support making Candidate an interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants