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/06/30 18:49:04 UTC
[GitHub] [geode] aaronlindsey opened a new pull request #6665: GEODE-9407: Handle exception getting region names
aaronlindsey opened a new pull request #6665:
URL: https://github.com/apache/geode/pull/6665
GetMemberInformationFunction may throw RegionDestroyedException if a
hosted region is destroyed while the function is checking the region
names. This exception is logged as an "error" level message which is
confusing to users.
Catch the exception and omit the destroyed region from the list of
hosted region names.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] aaronlindsey commented on a change in pull request #6665: GEODE-9407: Handle exception getting region names
Posted by GitBox <gi...@apache.org>.
aaronlindsey commented on a change in pull request #6665:
URL: https://github.com/apache/geode/pull/6665#discussion_r661885047
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/util/ManagementUtils.java
##########
@@ -162,14 +163,20 @@ public static DistributedMember getDistributedMemberByNameOrId(String memberName
Set<Region<?, ?>> rootRegions = cache.rootRegions();
for (Region<?, ?> rootRegion : rootRegions) {
- regionNames.add(rootRegion.getFullPath().substring(1));
+ try {
+ Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
- Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
+ for (Region<?, ?> subRegion : subRegions) {
+ regionNames.add(subRegion.getFullPath().substring(1));
+ }
- for (Region<?, ?> subRegion : subRegions) {
- regionNames.add(subRegion.getFullPath().substring(1));
+ } catch (RegionDestroyedException ignored) {
Review comment:
@jinmeiliao It's not the only exception this block can throw. I like your suggestion of catching general `Exception`. Would it be OK to log the caught exception as "debug" level? (My main purpose with this change is to prevent RegionDestroyedException from showing up in the logs under normal conditions when it is thrown from this function.)
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] aaronlindsey merged pull request #6665: GEODE-9407: Handle exception getting region names
Posted by GitBox <gi...@apache.org>.
aaronlindsey merged pull request #6665:
URL: https://github.com/apache/geode/pull/6665
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] aaronlindsey commented on a change in pull request #6665: GEODE-9407: Handle exception getting region names
Posted by GitBox <gi...@apache.org>.
aaronlindsey commented on a change in pull request #6665:
URL: https://github.com/apache/geode/pull/6665#discussion_r665632835
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/util/ManagementUtils.java
##########
@@ -162,14 +163,20 @@ public static DistributedMember getDistributedMemberByNameOrId(String memberName
Set<Region<?, ?>> rootRegions = cache.rootRegions();
for (Region<?, ?> rootRegion : rootRegions) {
- regionNames.add(rootRegion.getFullPath().substring(1));
+ try {
+ Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
- Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
+ for (Region<?, ?> subRegion : subRegions) {
+ regionNames.add(subRegion.getFullPath().substring(1));
+ }
- for (Region<?, ?> subRegion : subRegions) {
- regionNames.add(subRegion.getFullPath().substring(1));
+ } catch (RegionDestroyedException ignored) {
Review comment:
@jinmeiliao I changed this block of code to catch `Exception` instead of `RegionDestroyedException`, log a debug-level message, and then continue.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] aaronlindsey commented on pull request #6665: GEODE-9407: Handle exception getting region names
Posted by GitBox <gi...@apache.org>.
aaronlindsey commented on pull request #6665:
URL: https://github.com/apache/geode/pull/6665#issuecomment-871739005
The failing D-unit test is a known issue: https://issues.apache.org/jira/browse/GEODE-9070.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] aaronlindsey commented on a change in pull request #6665: GEODE-9407: Handle exception getting region names
Posted by GitBox <gi...@apache.org>.
aaronlindsey commented on a change in pull request #6665:
URL: https://github.com/apache/geode/pull/6665#discussion_r662638595
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/util/ManagementUtils.java
##########
@@ -162,14 +163,20 @@ public static DistributedMember getDistributedMemberByNameOrId(String memberName
Set<Region<?, ?>> rootRegions = cache.rootRegions();
for (Region<?, ?> rootRegion : rootRegions) {
- regionNames.add(rootRegion.getFullPath().substring(1));
+ try {
+ Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
- Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
+ for (Region<?, ?> subRegion : subRegions) {
+ regionNames.add(subRegion.getFullPath().substring(1));
+ }
- for (Region<?, ?> subRegion : subRegions) {
- regionNames.add(subRegion.getFullPath().substring(1));
+ } catch (RegionDestroyedException ignored) {
Review comment:
We are performing a health check using the REST API /members endpoint while some regions are being created and destroyed and RegionDestroyedException gets logged quite often. We would like to suppress this exception because it's expected for the call to subregions() to throw this exception after a region is destroyed and it's confusing to users to see this exception in the logs. I am hesitant to log it as an info level exception because it will still show up in the logs under normal circumstances and will still be confusing to users.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #6665: GEODE-9407: Handle exception getting region names
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #6665:
URL: https://github.com/apache/geode/pull/6665#discussion_r662435863
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/util/ManagementUtils.java
##########
@@ -162,14 +163,20 @@ public static DistributedMember getDistributedMemberByNameOrId(String memberName
Set<Region<?, ?>> rootRegions = cache.rootRegions();
for (Region<?, ?> rootRegion : rootRegions) {
- regionNames.add(rootRegion.getFullPath().substring(1));
+ try {
+ Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
- Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
+ for (Region<?, ?> subRegion : subRegions) {
+ regionNames.add(subRegion.getFullPath().substring(1));
+ }
- for (Region<?, ?> subRegion : subRegions) {
- regionNames.add(subRegion.getFullPath().substring(1));
+ } catch (RegionDestroyedException ignored) {
Review comment:
would "info" level be ok? not a big stack trace, just enough information about why some region didn't show up in the list.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] aaronlindsey commented on a change in pull request #6665: GEODE-9407: Handle exception getting region names
Posted by GitBox <gi...@apache.org>.
aaronlindsey commented on a change in pull request #6665:
URL: https://github.com/apache/geode/pull/6665#discussion_r661885047
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/util/ManagementUtils.java
##########
@@ -162,14 +163,20 @@ public static DistributedMember getDistributedMemberByNameOrId(String memberName
Set<Region<?, ?>> rootRegions = cache.rootRegions();
for (Region<?, ?> rootRegion : rootRegions) {
- regionNames.add(rootRegion.getFullPath().substring(1));
+ try {
+ Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
- Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
+ for (Region<?, ?> subRegion : subRegions) {
+ regionNames.add(subRegion.getFullPath().substring(1));
+ }
- for (Region<?, ?> subRegion : subRegions) {
- regionNames.add(subRegion.getFullPath().substring(1));
+ } catch (RegionDestroyedException ignored) {
Review comment:
It's not the only exception this block can throw. I like your suggestion of catching general `Exception`. Would it be OK to log the caught exception as "debug" level? (My main purpose with this change is to prevent RegionDestroyedException from showing up in the logs under normal conditions when it is thrown from this function.)
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #6665: GEODE-9407: Handle exception getting region names
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #6665:
URL: https://github.com/apache/geode/pull/6665#discussion_r661849690
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/util/ManagementUtils.java
##########
@@ -162,14 +163,20 @@ public static DistributedMember getDistributedMemberByNameOrId(String memberName
Set<Region<?, ?>> rootRegions = cache.rootRegions();
for (Region<?, ?> rootRegion : rootRegions) {
- regionNames.add(rootRegion.getFullPath().substring(1));
+ try {
+ Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
- Set<Region<?, ?>> subRegions = rootRegion.subregions(true);
+ for (Region<?, ?> subRegion : subRegions) {
+ regionNames.add(subRegion.getFullPath().substring(1));
+ }
- for (Region<?, ?> subRegion : subRegions) {
- regionNames.add(subRegion.getFullPath().substring(1));
+ } catch (RegionDestroyedException ignored) {
Review comment:
Are we sure this is the only exception this block of code can throw? If we want to prevent this block of code errors out, we should catch a general `Exception`, logs the exception, and then continue.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org