grpc-okhttp: implement OkHttpReadableBuffer readBytes(ByteBuffer dest)

[NguyenHoangSon96]

Proposed Changes

• Our project works with a library called arrow-java, and this library calls OkHttpReadableBuffer.readBytes(ByteBuffer dest) but this method was not implmented for grpc-okhttp so the calls failed. So I implemented this method so our project can work with arrow-java.
• I have a talk with the team why this method is not implemented Here

Changes

• Implement OkHttpReadableBuffer.readBytes(ByteBuffer dest).
• Make readToByteBufferShouldSucceed test runs for OkHttpReadableBuffer.

Additional Information

• Codecov complains about missing a test for throw new RuntimeException(e), I'm not sure if this line need to be tested.

[NguyenHoangSon96]

Hi @ejona86 , can you help me reviewing this PR.
This is my first time contribute for this repo, sorry If I did something incorrectly.
I can't add reviewers like other repos, so I tagged you.

[shivaspeaks]

I don't see there'd be any harm with your PR.

Should we go with something like this to align and be consistent with the rest of the codebase?

@Override
public void readBytes(ByteBuffer dest) {
  int remaining = dest.remaining();
  try {
    int read = buffer.read(dest);
    if (read < remaining) {
      throw new IndexOutOfBoundsException("EOF trying to read " + remaining + " bytes");
    }
  } catch (IOException e) {
    throw new RuntimeException(e);
  }
}

[NguyenHoangSon96]

Hi @shivaspeaks
Thank you for jumping on this so quickly, and thank you for your fix.
Can I ask how long the new version with this fix will be released?

[ejona86]

NAK. I don't mind the implementation, but the reason for the change is very, very bad.

Arrow is using our internals via reflection. This is not okay. Not only is it accessing internal classes, it is digging into its fields. In no way is that acceptable, and in no way are we going to assist in that.

This is an Arrow bug, and that code should be removed in Arrow.

There's no need to dig into our internals for accessing the ByteBuffer (which is what GetReadableBuffer reports to do). We have zero-copy APIs. They can use HasByteBuffer.getByteBuffer()+InputStream.skip() to loop through the ByteBuffers. If they want to access all the byte buffers simultaneously (as skip() will deallocate the last byte buffer, just like read() does), then they can use InputStream.mark(). If they want to take over ownership of the ByteBuffers, then they can use Detachable.detach().

[ejona86]

Closing in favor of deleting the method. #12580

[lidavidm]

Sorry about that. I've opened an issue to get rid of the sketchy code: apache/arrow-java#939

(Albeit, I'd rather recommend people use gRPC directly, I don't think having a gRPC wrapper in our code was really ever a good idea in the first place, but I don't think I can win that battle...)