Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
abstract tx5 backend in prep for m̶o̶c̶k̶ mem backend #105
abstract tx5 backend in prep for m̶o̶c̶k̶ mem backend #105
Changes from 3 commits
781686d
b93e474
681fd98
885af26
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How is this doing? Is it coming along at all or still not production ready?
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.
I wish I knew, but it doesn't seem to be the appropriate time to evaluate it. Once we have everything stable it'd be great to just go straight for the implementation so we can do a direct A/B comparison through our CI and manual usage.
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.
Are these already prevented from being enabled together?
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.
They are currently, but when we get to implementing it, I'd actually like them to not be exclusive, hence the fallback pattern in this default constructor. Not that we'd compile very often with both of them, but it would fit better into rust's features being additive paradigm.
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 could also be achieved by deriving
Default
. TheMock
variant becomes a bit ugly with listing all the features but I still think it's clearer (opinion) and removes the need for#[allow(unreachable_code)]
🙂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.
My hope was to make the backend features non-exclusive, hence the fallback returns.
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.
What happens in Callums suggestion when you compile with both features? Compiler error?
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 it would be a compiler error for multiple defined defaults. You could fix it with more
cfg_attr
like settingWebrtcRs
to only default ifgo-pion
isn't enabled:but I can imagine that this starts to look unruly as we possibly add more backends.
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.
What would the right interface look like here? Something like an
inner
that let's you get at the real connection and you have to know which ones it makes sense to ask about the use of webrtc as the caller?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.
What is this function for? Is it to check if it's connected or to check if it's using WebRTC? If it's the former then wouldn't
is_connected
be better?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.
It's the latter. "Connections" can communicate directly through sbd, then they attempt to upgrade to a webrtc connection. So
is_using_webrtc()
returns true once/if that upgrade is successful.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, that's still TBD. Does it even make sense to have this call be separate? Maybe it should just be part of the stats call, although that would just defer the problem, because there's an encapsulation question about that 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.
Maybe that's a different topic, but why does tx5 need to know whether it's connected indirectly through sbd or directly through webrtc?
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 far as I remember, it's informational or for unit tests. But I still submit this is something that doesn't have to be sorted out to merge this PR, since I'm not changing any functionality here.
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.
I asked out of curiosity, it wasn't meant as a subtle push to change it. I prefer being explicit about pushes ;-) Fine by me to keep as is.
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.
I would like it if we got a structure back based on the currently enabled back end. I was fine with tx5 vs tx2 just returning different data and I think the same is still true with go-pion vs webrtc-rs vs mock. Even if it is different based on the enabled back end, that's what I'd like to see presented up at the conductor level.
Maybe an enum could work and then the encapsulation is reasonable
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.
For sure that could work. Alternately, we could have this return serde_json::Value and have concrete types exposed that can be transcoded into if you know the backend you're using.
For now, I was just going to use this and the mock backend will return a blank ConnStats...
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.
Is there a reason to use trait objects instead of generics?
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.
Type-erasure. The associated types (or sub-generics) quickly get ridiculous with the stack of traits like this.
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.
Should this only be compile time configurable? Just thinking about writing tests against this, we'd have to build and test one way to run most tests and then build and test again with different features to use mocking
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.
Your question seems to answer why I wouldn't want to switch this to compile-time configuration. This way, most of the tests can use the true backend, but I can also have some which exercise the mock without having to have multiple top-level calls to
cargo test
with different features.