You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2021/04/16 12:01:12 UTC
[GitHub] [ratis] softgitron opened a new pull request #461: Ratis 952. Avoid NPE
softgitron opened a new pull request #461:
URL: https://github.com/apache/ratis/pull/461
## What changes were proposed in this pull request?
- Fix null pointer issues that were reported by SonarQube
- Fix spotty testInstallSnapshotNotificationCount test case
Test case fix is not strictly related to the NPE fixes, but I decided to add it to this pull request. For some reason this particular test failed especially often after these changes. However I couldn't find any direct connections with the NPE fixes and the test. I think the test itself is faulty and can be fixed with slight waiting before assertions.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-952
## How was this patch tested?
Patch was tested using existing tests. In addition, couple of testcases were added in to make sure that patches work as intended.
--
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] [ratis] szetszwo commented on pull request #461: Ratis 952. Avoid NPE
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #461:
URL: https://github.com/apache/ratis/pull/461#issuecomment-822610712
@softgitron , thanks a lot for contributing to Ratis!
> Test case fix is not strictly related to the NPE fixes, but I decided to add it to this pull request.
Please focus on a single aspect in each pull request. Please feel free to file multiple pull requests if there are different changes.
A single big pull request is hard to review and easy to make mistakes.
Indeed, it is not clear what does "Avoid NPE" mean. Which part (which class) of it try to fix? I suggest to start with a single class first.
--
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] [ratis] softgitron closed pull request #461: Ratis 952. Avoid NPE
Posted by GitBox <gi...@apache.org>.
softgitron closed pull request #461:
URL: https://github.com/apache/ratis/pull/461
--
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] [ratis] softgitron commented on pull request #461: Ratis 952. Avoid NPE
Posted by GitBox <gi...@apache.org>.
softgitron commented on pull request #461:
URL: https://github.com/apache/ratis/pull/461#issuecomment-821855424
If anyone got time to review my code, I'm open for suggestions and improvements. I tried my best to fix all the potential NPE issues without breaking anything critical, but the Ratis codebase is fairly complex so I may have made miss judgement at some point.
--
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] [ratis] szetszwo commented on pull request #461: Ratis 952. Avoid NPE
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #461:
URL: https://github.com/apache/ratis/pull/461#issuecomment-823308421
Yes, please file a JIRA per PR. Subtasks of RATIS-952 are good. Thanks!
--
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] [ratis] softgitron commented on pull request #461: Ratis 952. Avoid NPE
Posted by GitBox <gi...@apache.org>.
softgitron commented on pull request #461:
URL: https://github.com/apache/ratis/pull/461#issuecomment-822636980
> @softgitron , thanks a lot for contributing to Ratis!
>
> > Test case fix is not strictly related to the NPE fixes, but I decided to add it to this pull request.
>
> Please focus on a single aspect in each pull request. Please feel free to file multiple pull requests if there are different changes.
>
> A single big pull request is hard to review and easy to make mistakes.
>
> Indeed, it is not clear what does "Avoid NPE" mean. Which part (which class) of it try to fix? I suggest to start with a single class first.
Thanks for a reply @szetszwo! Yea, I admit that this PR ended up being way too fat, but the original fat Jira-ticket tempted me to do it this way. This PR actually fixes 8 different NPE issues in several classes reported by SonarQube and one flake test case.
I will close this PR for now and split it into 5-8 separate PRs in the following days. Would you also like to have separate Jira-tickets for all PRs or would sub tasks for RATIS-952 be sufficient (naturally flake test case should have its own Jira)?
--
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