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

multidim-interop: stop testing mplex #263

Closed
wants to merge 1 commit into from
Closed

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Aug 17, 2023

Kubo has dropped mplex support: ipfs/kubo#9958.

See libp2p/specs#553.

@marten-seemann marten-seemann linked an issue Aug 17, 2023 that may be closed by this pull request
@mxinden
Copy link
Member

mxinden commented Aug 17, 2023

I am very much in favor of removing mplex where ever possible.

That said, we still have users in the progress of moving off of mplex (e.g. libp2p/rust-libp2p#4334). Thus I find these interoperability tests still helpful.

Unless they are actively slowing us down (e.g. compile time, maintenance, ...) I suggest keeping them here until more folks moved off of mplex.

Thoughts?

@marten-seemann
Copy link
Contributor Author

It's definitely slowing down the runtime of the test suite. Not sure if we care enough about the speedup though.

The more important question is, what would we do when the test suite reveals any mplex failures? Are we going invest time into debugging and fixing those? From past experience, we have not, for a few months already. We've just disabled any test that failed with mplex.

@mxinden
Copy link
Member

mxinden commented Aug 17, 2023

The more important question is, what would we do when the test suite reveals any mplex failures? Are we going invest time into debugging and fixing those? From past experience, we have not, for a few months already. We've just disabled any test that failed with mplex.

The signal alone, i.e. knowing that our mplex implementations diverge, is helpful, e.g. might safe us some debugging work. I think we are on the same page when it comes to not wanting to invest into mplex.

Either way, not a strong preference.

@marten-seemann
Copy link
Contributor Author

Fair enough, I can see some value in that. I have a slight preference for dropping it now, but it's definitely not a strong opinion.

If we decide to keep it around though, I'd like us to define what the criteria are to drop it. Do you have any proposal for a timeline?

@mxinden
Copy link
Member

mxinden commented Aug 17, 2023

Off the top of my head I suggest:

  • Each implementation to drop support once further maintenance would require any significant amount of work.
  • Dropping support in test-plans/interop per implementation accordingly.

@marten-seemann
Copy link
Contributor Author

  • Each implementation to drop support once further maintenance would require any significant amount of work.

What counts as dropping? For example, in go-libp2p, the package still exists, but we'll deprecate it. That doesn't stop people from importing it, but if you do, you're on your own.

@mxinden
Copy link
Member

mxinden commented Aug 17, 2023

  • Each implementation to drop support once further maintenance would require any significant amount of work.

What counts as dropping? For example, in go-libp2p, the package still exists, but we'll deprecate it. That doesn't stop people from importing it, but if you do, you're on your own.

Maybe once removed from the go-libp2p monorepo and thus one can no longer include it in the go-libp2p interop binary?

@erwin-kok
Copy link

I'm currently in the process of adding kotlin-libp2p to the test-plans (I hope I get it working today/tomorrow).
My implementation currently only supports mplex. Others (yamux, quic) are planned, but takes a while.

I favor in deprecating mplex as well, however can we wait a bit longer before removing it from the test-plans?

@marten-seemann
Copy link
Contributor Author

@erwin-kok what’s your timeline?

@erwin-kok
Copy link

@erwin-kok what’s your timeline?

My first plan was to implement Quic right after I got the test-plans working. However, that is quite some work. Perhaps I can first implement Yamux. The only thing I know about Yamux is that it is not super complex. Just a wild guess: 3-4 weeks? Or is that too long?

@marten-seemann
Copy link
Contributor Author

Would you mind sharing why integrating QUIC would be a lot of work? Usually that’s the easiest transport to add.

@erwin-kok
Copy link

Would you mind sharing why integrating QUIC would be a lot of work? Usually that’s the easiest transport to add.

Sure. The only mature Quic implementation in Java/Kotlin that I could find is:
https://github.com/ptrd/kwik

So I need to:

  • Integrate this work into my implementation (Not sure how much time this is).
  • Update my implementation in such a way it also supports quic-multiaddresses and adding some mechanisms that for Quic, no security layer and muxer is needed (Not a lot of work).

Ok... How about this: I can spend a week or so (next week) trying to implement Quic in kotlin-libp2p. If by the end of next week it is not working yet, we can decide in what to do?

@erwin-kok
Copy link

Would you mind sharing why integrating QUIC would be a lot of work? Usually that’s the easiest transport to add.

Sure. The only mature Quic implementation in Java/Kotlin that I could find is: https://github.com/ptrd/kwik

So I need to:

  • Integrate this work into my implementation (Not sure how much time this is).
  • Update my implementation in such a way it also supports quic-multiaddresses and adding some mechanisms that for Quic, no security layer and muxer is needed (Not a lot of work).

Ok... How about this: I can spend a week or so (next week) trying to implement Quic in kotlin-libp2p. If by the end of next week it is not working yet, we can decide in what to do?

@mxinden
Or... put my other PR on hold (#264).
And continue with this PR once I finished implementing Quic. (However, I love to see some results whether my implementation is working against other implementations... ;-))

Perhaps this is the best, it also gives me more time to properly implement Quic.

@MarcoPolo
Copy link
Contributor

Why would we remove mplex from older versions? Those versions supported mplex when they came out.

To me, it makes sense to stop adding mplex on new versions going forward, but not removing their tests in prior released versions.

@p-shahi
Copy link
Member

p-shahi commented Aug 18, 2023

mplex still achieves better performance than yamux in js-libp2p (see ChainSafe/js-libp2p-yamux#42 and ChainSafe/js-libp2p-yamux#58) therefore I don't think it should be removed from js-libp2p just yet. Users are still reliant on it.

@marten-seemann
Copy link
Contributor Author

It also achieves better one-stream throughput than yamux in go-libp2p. This is not an argument to keep it though, given all the shortcomings around flow control / resource management.

@Jorropo
Copy link

Jorropo commented Aug 19, 2023

It also achieves better one-stream throughput than yamux in go-libp2p. This is not an argument to keep it though, given all the shortcomings around flow control / resource management.

I have an idea on how to fix it, need to spend a day or two but I don't think we care AFAIK.

@marten-seemann
Copy link
Contributor Author

Why would we remove mplex from older versions? Those versions supported mplex when they came out.

Fair enough. We can keep testing them until we remove those old versions from the interop test. This also means that @erwin-kok will be able to keep testing kotlin-libp2p without having to rush Yamux and / or QUIC.

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.

stop testing mplex
6 participants