Avro 1910 - HttpTransceiver buffering

[jcustenborder]

Corrected build errors of Avro.perf. Added ReadFully method to ensure that buffers are totally populated. Changed to stream 64k buffers instead of the entire buffer in one call.

[rdblue]

@jcustenborder, is there anyone else that is familiar with the C# code that can review this? I normally tag you for reviews. 😄

[rdblue]

@Simon24601, do you think you could review this one? We're short on reviewers for C#. And since you've contributed #131 it seems like you may want to review some of the other C# patches to get them in. Thanks!

[Simon24601]

I'll review this patch tonight.
Regards
Simon
On 31 Oct 2016 15:59, "Ryan Blue" [email protected] wrote:

@Simon24601 https://github.com/Simon24601, do you think you could
review this one? We're short on reviewers for C#. And since you've
contributed #131 #131 it seems like
you may want to review some of the other C# patches to get them in. Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#123 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIUPmn9jaNKhe2CSqETYuv82Kw-eZnjmks5q5hBagaJpZM4J6K85
.

[Simon24601]

@jcustenborder:
I've had a look at this patch, and I have a number of comments.
First off, I don't see any indication in the Avro Jira comments https://issues.apache.org/jira/browse/AVRO-1910 that this patch resolves the Jira. Did you receive any personal correspondence confirming this?
Second, I'm not really clear on what the main changes (i.e. the change to ReadBuffers(Stream inStream, byte[] intBuffer)) are supposed to achieve. Currently, each message is assigned a buffer of the appropriate length, the full message is read and then the buffer is wrapped in a MemoryStream. With your change, we have a smaller buffer, but we copy the data into a MemoryStream, which will have its own internal buffer. So we marginally increase memory usage, but otherwise the function hasn't really changed - it is still a blocking function that must read every message completely into memory before it returns. What am I missing?
Next, the ReadFully method seems a little strange. Typically (imo) a buffer needs to be at least as big as the message it receives, not exactly the same size. So a ReadFully method would take the Stream, buffer and an integer, corresponding to the requested number of bytes to read. Or maybe no integer, but it reads until it receives a termination character in the stream. But not just reading until the buffer is exhausted. Currently, this method is safe, because it's only used in ReadInt, and is passed a 4-byte buffer, but it still feels strange.
I'm not enthusiastic about making ReadFully and ReadInt public just so that they can be tested, but this is the sort of thing that leads to flamewars, so I'm not going to object strongly to that. I'd just prefer them to be tested by being used in the methods that are already public.
Your deletion of "-framework=4.0" from build.sh means that it no longer builds using the docker container that ships with this project, on my computer anyway (CentOS 7). Do you build from inside Mono or do you use the docker scripts?
I also get the test failure that was discussed in the Jira comments. I'm currently debugging this to figure out what's going wrong. Increasing the timeout time doesn't seem to fix it. I know you saw that it was reading past the end of the schema, but I don't see why it would do that. From what I can tell, it stalls inside HttpClientServerTest::Send(GenericRecord message). I don't think we can write it off as an unimportant test failure until we can track down why it is different.
Thanks for adding apache/perf/app.config - it's nice that the solution builds straight out of the box.

I'll let you know if I can track down the origin of the test failure. In the meantime, I guess the most important question is whether this solves the issue that was originally reported (and if the bug-reporter can strip it down to a useful test, that's even better).

[jcustenborder]

Thanks @Simon24601

First off, I don't see any indication in the Avro Jira comments https://issues.apache.org/jira/browse/AVRO-1910 that this patch resolves the Jira. Did you receive any personal correspondence confirming this?

The user has gone dark, so I don't have a confirmation.

Second, I'm not really clear on what the main changes (i.e. the change to ReadBuffers(Stream inStream, byte[] intBuffer)) are supposed to achieve. Currently, each message is assigned a buffer of the appropriate length, the full message is read and then the buffer is wrapped in a MemoryStream. With your change, we have a smaller buffer, but we copy the data into a MemoryStream, which will have its own internal buffer. So we marginally increase memory usage, but otherwise the function hasn't really changed - it is still a blocking function that must read every message completely into memory before it returns. What am I missing?

In the users use case they were bringing in very large messages that were being handed to a read call. It was on the order of several megabytes. int num = inStream.Read(buffer, offset, count); will return the data available, not necessarily the entire message. I moved to use a 64K buffer and multiple calls to ensure that all of the data was read.

Next, the ReadFully method seems a little strange. Typically (imo) a buffer needs to be at least as big as the message it receives, not exactly the same size. So a ReadFully method would take the Stream, buffer and an integer, corresponding to the requested number of bytes to read. Or maybe no integer, but it reads until it receives a termination character in the stream. But not just reading until the buffer is exhausted. Currently, this method is safe, because it's only used in ReadInt, and is passed a 4-byte buffer, but it still feels strange.

ReadFully is just a similar implementation to this. DataInputStream.readFully(byte[])

I'm not enthusiastic about making ReadFully and ReadInt public just so that they can be tested, but this is the sort of thing that leads to flamewars, so I'm not going to object strongly to that. I'd just prefer them to be tested by being used in the methods that are already public.
Your deletion of "-framework=4.0" from build.sh means that it no longer builds using the docker container that ships with this project, on my computer anyway (CentOS 7). Do you build from inside Mono or do you use the docker scripts?

I was using the running the build directly on my mac and it errors unless that was removed.

I also get the test failure that was discussed in the Jira comments. I'm currently debugging this to figure out what's going wrong. Increasing the timeout time doesn't seem to fix it. I know you saw that it was reading past the end of the schema, but I don't see why it would do that. From what I can tell, it stalls inside HttpClientServerTest::Send(GenericRecord message). I don't think we can write it off as an unimportant test failure until we can track down why it is different.
Thanks for adding apache/perf/app.config - it's nice that the solution builds straight out of the box.

I wouldn't mind holding off on this pull and putting in a few more that would increase the logging in this section of code. The unit tests are not logging anything when an exception is raised.

In the future would you mind dropping the comments on the files page? It makes it much easier to keep track of what you are pointing to.

[Simon24601]

I think that this will be a problem, because the solution needs to build cleanly within the docker container out-of-the-box and this prevents it doing so. Could you remove this from this pull request, or amend something so it still works (I say "something" because I'm not sure if this requires a change to the docker setup or the solution file).

[jcustenborder]

I can put that back in. It would not work when building directly from my Mac.

[Simon24601]

@jcustenborder:
OK, the reason behind the change is much clearer - thanks for clarifying
it. And while ReadFully still surprises me, it makes a lot more sense if I
think about it from the perspective of limited resources - the buffer can
get allocated once you know how much data is expected. So I'm happy with
most of the above.

I'll add further comments on the files page - sorry, I've never used a code
review commenting system that wasn't extremely slow before, so I tend to
reply using emails.

On Fri, Nov 4, 2016 at 9:27 PM, Jeremy Custenborder <
[email protected]> wrote:

Thanks @Simon24601 https://github.com/Simon24601

First off, I don't see any indication in the Avro Jira comments
https://issues.apache.org/jira/browse/AVRO-1910 that this patch resolves
the Jira. Did you receive any personal correspondence confirming this?

The user has gone dark, so I don't have a confirmation.

Second, I'm not really clear on what the main changes (i.e. the change to
ReadBuffers(Stream inStream, byte[] intBuffer)) are supposed to achieve.
Currently, each message is assigned a buffer of the appropriate length, the
full message is read and then the buffer is wrapped in a MemoryStream. With
your change, we have a smaller buffer, but we copy the data into a
MemoryStream, which will have its own internal buffer. So we marginally
increase memory usage, but otherwise the function hasn't really changed -
it is still a blocking function that must read every message completely
into memory before it returns. What am I missing?

In the users use case they were bringing in very large messages that were
being handed to a read call. It was on the order of several megabytes. int
num = inStream.Read(buffer, offset, count); will return the data
available, not necessarily the entire message. I moved to use a 64K buffer
and multiple calls to ensure that all of the data was read.

Next, the ReadFully method seems a little strange. Typically (imo) a
buffer needs to be at least as big as the message it receives, not exactly
the same size. So a ReadFully method would take the Stream, buffer and an
integer, corresponding to the requested number of bytes to read. Or maybe
no integer, but it reads until it receives a termination character in the
stream. But not just reading until the buffer is exhausted. Currently, this
method is safe, because it's only used in ReadInt, and is passed a 4-byte
buffer, but it still feels strange.

ReadFully is just a similar implementation to this.
DataInputStream.readFully(byte[])
https://docs.oracle.com/javase/7/docs/api/java/io/DataInputStream.html#readFully(byte%5B%5D)

I'm not enthusiastic about making ReadFully and ReadInt public just so
that they can be tested, but this is the sort of thing that leads to
flamewars, so I'm not going to object strongly to that. I'd just prefer
them to be tested by being used in the methods that are already public.
Your deletion of "-framework=4.0" from build.sh means that it no longer
builds using the docker container that ships with this project, on my
computer anyway (CentOS 7). Do you build from inside Mono or do you use the
docker scripts?

I was using the running the build directly on my mac and it errors unless
that was removed.

I also get the test failure that was discussed in the Jira comments. I'm
currently debugging this to figure out what's going wrong. Increasing the
timeout time doesn't seem to fix it. I know you saw that it was reading
past the end of the schema, but I don't see why it would do that. From what
I can tell, it stalls inside HttpClientServerTest::Send(GenericRecord
message). I don't think we can write it off as an unimportant test failure
until we can track down why it is different.
Thanks for adding apache/perf/app.config - it's nice that the solution
builds straight out of the box.

I wouldn't mind holding off on this pull and putting in a few more that
would increase the logging in this section of code. The unit tests are not
logging anything when an exception is raised.

In the future would you mind dropping the comments on the files
https://github.com/apache/avro/pull/123/files page? It makes it much
easier to keep track of what you are pointing to.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#123 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIUPmphjbZeqQr_FKNBZlEiMZCSrPpw3ks5q66M-gaJpZM4J6K85
.

[Simon24601]

This line sometimes stalls and causes the timeout failure in the HttpClientServerTest.cs that was mentioned in the Jira ticket comments. I think that changing it to
while ((length > totalRead)&&(read = inStream.Read(buffer, 0, Math.Min(length - totalRead, buffer.Length))) > 0)
prevents the timeout on this line. But the test doesn't pass - it just times out later.