=str Skip parsing when buffer size < 1 #363
Conversation
He-Pin
requested review from
jrudolph,
an-tex,
mdedetrich,
pjfanning and
jxnu-liguobin
There was a problem hiding this comment.
This might be considered excessive but considering we have had a regression in this area of Pekko, would it be possible to add a unit test which checks that when the buffer size is 0 to check that both current Pekko and the change in this PR both return false?
The easiest way to do this is to make seekObject() package private (so you can access it in a test) and construct JsonObjectParser (which initialises buffer to empty so buffer.length is zero) and immediately call seekObject() to see if it returns false.
If this is too difficult then let me know and I can reconsider.
|
It can be false if the downstream poll, but the upstream is not ready, or downstream is faster and upstream is idle. I can add more detailed test for JsonFramming but I don't see much value to add a test for the short-circuit logic，as it's identical to the objectCompleted when the buffer is empty As the chances this short-circuit can benefit, i think that maybe why it's not there in the firsr place. |
|
tbh I am not entirely familiar with this section of Pekko, I am just being cautious/skeptical because we had a general regression here which I am trying to avoid in the future. If you are happy with adding a more detailed |
|
Could we do this in v1.0.1? It's not a bug and it just doesn't feel urgent. |
|
Agreed |
|
let's defer this. i want to add some random tests against JsonFraming. |
Perfect thanks! Such tests should definitely be merged before 1.0.0 if they are ready in time to give us extra confidence. |
|
@He-Pin I am adding this to 1.1.x milestone |
stream/src/main/scala/org/apache/pekko/stream/impl/JsonObjectParser.scala
Outdated
Show resolved
Hide resolved
He-Pin
changed the title
| @@ -115,20 +115,21 @@ import pekko.util.ByteString | |||
| private def seekObject(): Boolean = { | |||
| completedObject = false | |||
| val bufSize = buffer.length | |||
| if (bufSize > 1) { | |||
There was a problem hiding this comment.
a {} is minimal 2 bytes and need to skip the blank char too
He-Pin
changed the title
He-Pin
force-pushed
the
jsonObject
branch
2 times, most recently
from
c1bbfbb
to
0492ad5
Compare
|
The error is unrelated. |
I think we can do a quic returning if the buffer size is zero.