You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "He-Pin (via GitHub)" <gi...@apache.org> on 2023/05/29 16:29:24 UTC
[GitHub] [incubator-pekko] He-Pin opened a new pull request, #363: =str Skip parsing when buffer size is 0.
He-Pin opened a new pull request, #363:
URL: https://github.com/apache/incubator-pekko/pull/363
I think we can do a quic returning if the buffer size is zero.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #363: =str Skip parsing when buffer size is 0.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#issuecomment-1567375348
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 `JsonFraming` test that would be great, I suspect you would do this in a separate PR? If so let me know and can review this in more detail in the next couple of days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin merged pull request #363: =str Skip parsing when buffer size < 1
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin merged PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #363: =str Skip parsing when buffer size is 0.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#discussion_r1285179109
##########
stream/src/main/scala/org/apache/pekko/stream/impl/JsonObjectParser.scala:
##########
@@ -115,7 +115,9 @@ import pekko.util.ByteString
private def seekObject(): Boolean = {
completedObject = false
val bufSize = buffer.length
-
+ if (bufSize == 0) {
Review Comment:
I don't like 'return' statements. Can't you just do this ?
```
if (bufSize > 0) {
// add everything from the rest if the function here
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #363: =str Skip parsing when buffer size < 1
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#discussion_r1291553446
##########
stream/src/main/scala/org/apache/pekko/stream/impl/JsonObjectParser.scala:
##########
@@ -115,20 +115,21 @@ import pekko.util.ByteString
private def seekObject(): Boolean = {
completedObject = false
val bufSize = buffer.length
+ if (bufSize > 1) {
Review Comment:
a `{}` is minimal 2 bytes and need to skip the blank char too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #363: =str Skip parsing when buffer size is 0.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#issuecomment-1666778939
@He-Pin I am adding this to 1.1.x milestone
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #363: =str Skip parsing when buffer size < 2
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#discussion_r1291553446
##########
stream/src/main/scala/org/apache/pekko/stream/impl/JsonObjectParser.scala:
##########
@@ -115,20 +115,21 @@ import pekko.util.ByteString
private def seekObject(): Boolean = {
completedObject = false
val bufSize = buffer.length
+ if (bufSize > 1) {
Review Comment:
a `{}` is minimal 2 bytes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #363: =str Skip parsing when buffer size < 1
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#issuecomment-1675131550
The error is unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #363: =str Skip parsing when buffer size is 0.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#issuecomment-1567438097
let's defer this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #363: =str Skip parsing when buffer size is 0.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#issuecomment-1567373295
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.
As the chances this short-circuit can benefit, i think that maybe why it's not there in the firsr place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #363: =str Skip parsing when buffer size is 0.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#issuecomment-1568004230
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #363: =str Skip parsing when buffer size is 0.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#issuecomment-1567379546
Could we do this in v1.0.1? It's not a bug and it just doesn't feel urgent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #363: =str Skip parsing when buffer size is 0.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#issuecomment-1567405558
Agreed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #363: =str Skip parsing when buffer size is 0.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#discussion_r1285179109
##########
stream/src/main/scala/org/apache/pekko/stream/impl/JsonObjectParser.scala:
##########
@@ -115,7 +115,9 @@ import pekko.util.ByteString
private def seekObject(): Boolean = {
completedObject = false
val bufSize = buffer.length
-
+ if (bufSize == 0) {
Review Comment:
I don't like 'return' statements. Can't you just do this ?
```
if (bufSize > 0) {
// add everything from the rest of the function here
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] jrudolph commented on a diff in pull request #363: =str Skip parsing when buffer size is 0.
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #363:
URL: https://github.com/apache/incubator-pekko/pull/363#discussion_r1291132471
##########
stream/src/main/scala/org/apache/pekko/stream/impl/JsonObjectParser.scala:
##########
@@ -115,7 +115,9 @@ import pekko.util.ByteString
private def seekObject(): Boolean = {
completedObject = false
val bufSize = buffer.length
-
+ if (bufSize == 0) {
Review Comment:
Agreed, maybe not super important in that case, but let's avoid a potential exception being thrown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org