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

Reorganize NettyBufferProvider, NettyByteBuf and improve docs #1149

Conversation

stIncMale
Copy link
Member

@stIncMale stIncMale commented Jun 22, 2023

I noticed a few things that are worth changing while working on the gRPC POC:

  • NettyBufferProvider is production code, but is used only in a single test class.
  • NettyByteBuf is package-access in an API package, bit in the POC I need access to it from a different package, so had to move it as public into an internal package.
  • NettyByteBuf stores a reference to the proxied buffer in the heap memory but does not retain the proxied buffer. At the very least that should be documented.

@stIncMale stIncMale requested a review from jyemin June 22, 2023 23:37
@stIncMale stIncMale self-assigned this Jun 22, 2023
@jyemin
Copy link
Collaborator

jyemin commented Jun 23, 2023

The ByteBuf abstraction is ... not good. It's something I've wanted to look at for a while but there hasn't been a good reason to mess with it.

@stIncMale
Copy link
Member Author

I don't currently have an opinion on either our ByteBuf, or the Java SE one, or the one in the Netty API. But I understand why our own ByteBuf was introduced (to abstract over the Java SE API and Netty API), and I read in "Netty in action" why io.netty.buffer.ByteBuf was introduced instead of using java.nio.ByteBuffer (it's impossible to extend java.nio.ByteBuffer, which makes it impossible to implement a composite buffer or add reference counting).

@stIncMale stIncMale merged commit ca5271d into mongodb:master Jun 23, 2023
@stIncMale stIncMale deleted the reorganize_NettyBufferProvider_NettyByteBuf_improve_internal_docs branch June 23, 2023 04:18
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.

2 participants