Skip to content

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