You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/04/21 08:05:07 UTC
[GitHub] [zookeeper] pkuwm opened a new pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
pkuwm opened a new pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683
### Problem
Resolves https://issues.apache.org/jira/browse/ZOOKEEPER-4281
There are sanity checks for packet size when deserializing a packet. One place has the inclusion: len >= packetLen. It rejects to deserialize the bytes that are exactly sized jute.maxbuffer. It's confusing. This check should use ">" so the maxbuffer length packet can still be deserialized, like the other checks.
```
if (len < 0 || len >= packetLen) {
throw new IOException("Packet len " + len + " is out of range!");
}
```
### Solution
Change: `len >= packetLen` -> `len > packetLen`
```
if (len < 0 || len > packetLen) {
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-819314322
It makes sense to me
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-823005161
@eolivelli @arshadmohammad @maoling Is it fine to merge this PR? Let me know if there is anything else we should do before merging it. Thank you!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-823865971
it looks like the link to GH actions diappeared
btw this fix is save and main QA checks passed.
merging it now
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-819867536
Thanks for the review, @eolivelli, @arshadmohammad.
Failed CI checks don't seem related to this change.
retest maven build
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-818973192
@eolivelli @maoling @ztzg @hanm Could you help take a quick look and decide whether the PR is valid/helpful? Thank you!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-821627598
> Sorry for late reply.
>
> Can you please add one unit test around the code you changed? In order to not introduce regressions in the future
@eolivelli Thanks for the suggestion. A unit test is added :) Could you take another look?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli closed pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-823856129
I have restarted the CI job, we still have a few problems with GitHub actions and JDK11, sorry
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-819927777
retest maven build
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-823868265
committed to master branch (3.8.x) and to branch-3.7 (3.7.1)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-823027691
I have restarted CI jobs, as soon as CI is green I will merge.
sorry, it is taking so long
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm removed a comment on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
pkuwm removed a comment on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-819927777
retest maven build
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm commented on pull request #1683: ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1683:
URL: https://github.com/apache/zookeeper/pull/1683#issuecomment-823799501
> I have restarted CI jobs, as soon as CI is green I will merge.
> sorry, it is taking so long
@eolivelli thanks for triggering the checks. It seems a check (tests) timed out. From the previous merged PRs, the tests are not stable. It might need several retries to get all checks green :(
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org