You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/03/12 03:14:11 UTC
[GitHub] [geode] moleske opened a new pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
moleske opened a new pull request #6125:
URL: https://github.com/apache/geode/pull/6125
- make a visible for testing function that takes in filestreams of
netstats and stats
- since it is an integration test anyway, just read real files from test
resources
- use variable name `ignore` for exceptions we catch but don't do
anything with
- remove unneeded initialization of String
- remove empty if clause
- remove unneeded assignments
- simplify condition check
- replace explicit type with <>
Was getting the ruby taste out of my mouth from another project and thought I'd tackle another powermock removal.
Anyone who has push access to the apache/geode repo can push changes my branch, so please do so if you see changes that need to happen
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
There's a commit with the actual change and a follow up with cleaning up some warnings. Who ever merges may squash into one.
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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] [geode] moleske commented on pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #6125:
URL: https://github.com/apache/geode/pull/6125#issuecomment-797718730
I don't understand the failures, could use some help. I think the unit test one actually passed but failed to update properly. The stress test one is confusing me and can't figure out what it is trying to communicate
----------------------------------------------------------------
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] [geode] moleske commented on a change in pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #6125:
URL: https://github.com/apache/geode/pull/6125#discussion_r593364495
##########
File path: geode-core/src/main/java/org/apache/geode/internal/statistics/platform/LinuxProcFsStatistics.java
##########
@@ -156,14 +152,24 @@ public static void refreshProcess(int pid, LocalStatisticsImpl stats) {
}
public static void refreshSystem(LocalStatisticsImpl stats) {
+ try {
+ refreshSystem(stats, new FileInputStream("/proc/stat"),
+ new FileInputStream("/proc/net/netstat"));
Review comment:
I ended up changing refreshSystem to take in file paths to have only one try/catch. There was a scenario I thought of where if I kept the try and it failed to FileInputStream, it wouldn't update the other stats. The original code would attempt to update the other stats if the FileInputStream failed
----------------------------------------------------------------
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] [geode] DonalEvans commented on pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #6125:
URL: https://github.com/apache/geode/pull/6125#issuecomment-797724805
> I don't understand the failures, could use some help. I think the unit test one actually passed but failed to update properly. The stress test one is confusing me and can't figure out what it is trying to communicate
I think you may need to rebase your branch on the latest develop. There's a commit (since reverted) that was causing those failures in various DUnit tests
----------------------------------------------------------------
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] [geode] moleske commented on pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #6125:
URL: https://github.com/apache/geode/pull/6125#issuecomment-797767696
> > I don't understand the failures, could use some help. I think the unit test one actually passed but failed to update properly. The stress test one is confusing me and can't figure out what it is trying to communicate
>
> I think you may need to rebase your branch on the latest develop. There's a commit (since reverted) that was causing those failures in various DUnit tests
Cool thanks @DonalEvans, will see how it goes 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] [geode] DonalEvans commented on a change in pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6125:
URL: https://github.com/apache/geode/pull/6125#discussion_r593368678
##########
File path: geode-core/src/main/java/org/apache/geode/internal/statistics/platform/LinuxProcFsStatistics.java
##########
@@ -156,14 +152,24 @@ public static void refreshProcess(int pid, LocalStatisticsImpl stats) {
}
public static void refreshSystem(LocalStatisticsImpl stats) {
+ try {
+ refreshSystem(stats, new FileInputStream("/proc/stat"),
+ new FileInputStream("/proc/net/netstat"));
Review comment:
Ah, yeah, that's a nicer approach
----------------------------------------------------------------
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] [geode] moleske merged pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
moleske merged pull request #6125:
URL: https://github.com/apache/geode/pull/6125
----------------------------------------------------------------
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] [geode] moleske commented on pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #6125:
URL: https://github.com/apache/geode/pull/6125#issuecomment-799734242
@mhansonp @kirklund looks like it is finally green, sorry that took awhile (still don't really know what was wrong with the stress test 🤷♀️)
----------------------------------------------------------------
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] [geode] moleske commented on a change in pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #6125:
URL: https://github.com/apache/geode/pull/6125#discussion_r593322823
##########
File path: geode-core/src/main/java/org/apache/geode/internal/statistics/platform/LinuxProcFsStatistics.java
##########
@@ -156,14 +152,24 @@ public static void refreshProcess(int pid, LocalStatisticsImpl stats) {
}
public static void refreshSystem(LocalStatisticsImpl stats) {
+ try {
+ refreshSystem(stats, new FileInputStream("/proc/stat"),
+ new FileInputStream("/proc/net/netstat"));
Review comment:
That seems reasonable. The main reason I thought about just passing file path is that would consolidate everything back to one try statement
----------------------------------------------------------------
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] [geode] moleske commented on pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #6125:
URL: https://github.com/apache/geode/pull/6125#issuecomment-797219668
welp don't want to introduce resource leaks. Will try maybe just passing file path instead maybe?
----------------------------------------------------------------
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] [geode] lgtm-com[bot] commented on pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6125:
URL: https://github.com/apache/geode/pull/6125#issuecomment-797216672
This pull request **introduces 2 alerts** when merging e956f5fb55826f84868fb0224d4dcc44dca7065f into 00c0e7980bdead3995fc352c3e0f3f52cce4e4fb - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-ad5f45ef2f5fb25382e527b8563411835ce8ffa6)
**new alerts:**
* 2 for Potential input resource leak
----------------------------------------------------------------
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] [geode] moleske commented on pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #6125:
URL: https://github.com/apache/geode/pull/6125#issuecomment-799844313
> This test uses resources as files. Sometimes it's easiest to copy from src/resources to a TemporaryFolder. **`geode-junit/src/main/java/org/apache/geode/test/util/ResourceUtils.java`** is all about helping you do things like this. Take a look at it sometime and see if it makes tests like this easier for you.
Thanks for sharing that! I think it confused me more at first cause I'm used to loading test files from resources, so didn't think about a `TemporaryFolder`. If folks in the geode community have a strong opinion I can change, but I'm inclined to leave it as.
----------------------------------------------------------------
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] [geode] DonalEvans commented on a change in pull request #6125: GEODE-6143: remove powermock from LinuxSystemStatsTest
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6125:
URL: https://github.com/apache/geode/pull/6125#discussion_r592908308
##########
File path: geode-core/src/main/java/org/apache/geode/internal/statistics/platform/LinuxProcFsStatistics.java
##########
@@ -156,14 +152,24 @@ public static void refreshProcess(int pid, LocalStatisticsImpl stats) {
}
public static void refreshSystem(LocalStatisticsImpl stats) {
+ try {
+ refreshSystem(stats, new FileInputStream("/proc/stat"),
+ new FileInputStream("/proc/net/netstat"));
Review comment:
I think that the potential resource leaks here can be fixed by using try with resources:
```
try (FileInputStream procStat = new FileInputStream("/proc/stat");
FileInputStream netStat = new FileInputStream("/proc/net/stat")) {
refreshSystem(stats, procStat, netStat);
}
```
----------------------------------------------------------------
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