Modify ByteBufferReadableStreamChannel to take in a list of ByteBuffer about ambry HOT 10 OPEN

Hey @vgkholla i'm a total newbie looking to get involved with the project! This looks like a great intro point do you mind if i throw up a pr?

from ambry.

@alecharmon Sure, go ahead !

If you want guidelines on what we expect on a PR, please take a look at the older ones. Usually we expect unit tests that cover all the lines (and if-else cases) of the new code and a confirmation of successful build (./gradlew build && ./gradlew test).

Let me know if you need any other assistance.

from ambry.

++

from ambry.

@alecharmon If you are working on this, would make sense to wait until #460 is merged and #391 is closed. Will make the job easier.

from ambry.

ahh ok tx @vgkholla

from ambry.

@alecharmon PR has been merged and the issue closed

from ambry.

If the issue is still open, I would like to work on this.

I also wanted to get more details -

1. What is the usecase for the new constructor? Is it to create thebuffer thats part of the class from a list of buffers? Or is it to support a new list of buffers in the class?

2. Could you please expand on this -

   (The first constructor would call into the second constructor to avoid duplicate code).

Is the intention to just create a single buffer whose capacity equals the sum of the length of all buffers in the passed list?

from ambry.

@moontails the class surely should not create copies of any buffers it receives. The use case for the new constructor is that you might want to expose a List of ByteBuffer as a ReadableStreamChannel. The current implementation forces the user to copy all that data into one big ByteBuffer which can be avoided. Also the current implementation does not allow for dynamically adding more data to the channel.

The class internally would do something like:

1. Write all available ByteBuffer out to the channel on readInto.
2. Wait for all write callbacks to arrive and then set read futureand call read callback.

A very similar piece of code is available in MockRestRequest (link). Look at how its readInto() method is implemented and that should give you an idea. I would think that the code here should be quite similar here but you will have to add tests.

Also, like in MockRestRequest, you should provide an addContent() method that helps write data dynamically instead of having to provide it all in the beginning. This is what will be eventually helpful.

As for reusing code, the first constructor would do something like

public ByteBufferReadableStreamChannel(ByteBuffer buffer) {
  this(Collections.singletonList(buffer));
}

Let me know if you have any more questions.

from ambry.

@vgkholla I misinterpreted the quoted statement.

Thank you for providing more explanation, it gives me a better picture. I will take a crack at this.

from ambry.

@vgkholla would it be alright to track addContent() in a separate issue? I would like more details on its expected behavior for read operations.

from ambry.