ZOOKEEPER-3709: Pre-defined the size of ByteArrayOutputStream#1350
Open
kaansonmezoz wants to merge 11 commits intoapache:masterfrom
Open
ZOOKEEPER-3709: Pre-defined the size of ByteArrayOutputStream#1350kaansonmezoz wants to merge 11 commits intoapache:masterfrom
kaansonmezoz wants to merge 11 commits intoapache:masterfrom
Conversation
Member
|
Link to PR#1267|ZOOKEEPER-3709 |
Author
|
@maoling What do you mean by "link to" ? It's already closed, not merged so that issue is still active, isn't it? |
symat
requested changes
Jun 8, 2020
Contributor
symat
left a comment
symat
left a comment
There was a problem hiding this comment.
@kaansonmezoz thanks for trying to improve the ZooKeeper code!
I noticed that there are 3-4 other places just in Learner.java where the code could be improved in the similar ways, where ByteArrayOutputStream is currently created without specifying the size. Is there a reason why you pick only the request method to fix? Would you mind go through all these places? Maybe even checking if we have the similar issue in other classes? It would be nice to have these things consistently used across the code base.
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
Show resolved
Hide resolved
Contributor
|
I also changed the PR title to include the apache issue ID. |
symat
changed the title
Reformatted If statement
symat
requested changes
Jul 5, 2020
Contributor
symat
left a comment
symat
left a comment
There was a problem hiding this comment.
looks good to me, thanks for taking the time on this PR!
According to Travis, you have one more small checkstyle violation.
| // size of sessionId, cxId and type in bytes | ||
| int size = Long.BYTES + 2 * Integer.BYTES; | ||
| byte[] bytes = null; | ||
| if(request.request != null) { |
Contributor
There was a problem hiding this comment.
you need a space after the 'if' statement to fix the checkstyle error.
(btw you can run checkstyle locally by: mvn verify spotbugs:check checkstyle:check -DskipTests )
Author
There was a problem hiding this comment.
Oh man thanks a lot. I thought I fixed that one but it turns out there is another one out there :(
Contributor
|
also you accidentally committed this file: (this is a known irritating thing in ZooKeeper, some tests are changing this resource... as far as I remember maybe @maoling is already checking how to fix this) |
This reverts commit a0ad512.
Author
|
I know it's been awhile since the last time committed. Could you please review again @symat :) |
Contributor
|
Thanks for working on this! :) |
Author
Yeah I know that feeling 🙁. It's totally fine man, I was also in a similar position, that's why I hadn't contributed to this issue 🙁 |
Contributor
|
Thanks, it looks good to me now. But there is a unit test failing ( |
Contributor
|
retest maven build |
Contributor
|
I tried it locally on my machine too. The same test runs for me on the master branch, but fails when I apply your patch. Please take a look. |
|
Hello @symat, |
Contributor
|
Hi @ruiyang00 , this PR never got merged. As far as I can tell based on the comments, the PR caused some unit tests to fail, so I asked you to take a look about this failure:
I don't see the CI jobs anymore (I guess Jenkins cleaned the logs long time ago). Anyway, a lot of changes happened on the ZooKeeper master since last August, so please rebase your PR, that will also trigger a new CI run. |
|
Hello @symat, I am on it. One question regarding the zookeeper's unit test system. When we make some changes to the zookeeper-server package, do we only run the unit test under that package or do we run the I pull directly from the master branch Yesterday. |
Contributor
|
Unfortunately we are facing some issues with the stability of the unit tests, the community is currently working on the stabilization of the tests on the master branch before the 3.7.0 release. Asking maven to run the tests sequentially might help in the meanwhile, e.g.: But my suggestion would be to do the rebase on your feature branch, then check which tests are failing by the CI and then you only need to re-run those few tests locally to check if they work or not... e.g.: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.