You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/09/29 08:41:56 UTC
[GitHub] [ozone] sumitagrawl opened a new pull request, #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
sumitagrawl opened a new pull request, #3787:
URL: https://github.com/apache/ozone/pull/3787
## What changes were proposed in this pull request?
1. Cleanup for RatisDropwizardExports registry on stop/shutdown
2. Avoiding continuous loop after interrupt for DeleteBlocksCommandHandler
These is observed in Ozone integration test
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7271
## How was this patch tested?
This is verified running ozone integration test and verifying heap dump for same. This issue is not observed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r989850485
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMBlockDeletingService.java:
##########
@@ -229,7 +229,7 @@ public String getServiceName() {
@Override
public void stop() {
- throw new RuntimeException("Not supported operation.");
+ // Not supported operation
Review Comment:
In other words it's not an unsupported operation, rather a no-op.
`BlockManagerImpl` explicitly starts and shuts down `SCMBlockDeletingService`, which is also registered with `SCMServiceManager`, which manages start/stop of registered services. I think it's confusing to have both of these lifecycle management calls for the same service. Can we merge `stop` and `shutdown` and let only `SCMServiceManager` manage it?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
sumitagrawl commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r985353464
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/BackgroundSCMService.java:
##########
@@ -126,7 +126,6 @@ public void stop() {
log.info("{} Service is not running, skip stop.", getServiceName());
return;
}
- backgroundThread.interrupt();
Review Comment:
When current stopping thread calls interrupt, its interrupted and further handling is stopped throwing interrupt exception. This is causing to stop to fail.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMBlockDeletingService.java:
##########
@@ -229,7 +229,7 @@ public String getServiceName() {
@Override
public void stop() {
- throw new RuntimeException("Not supported operation.");
+ // Not supported operation
Review Comment:
When call stop of serviceManager, as its registered part of serviceManager causes stop to fail. We must support stop as common framework or do nothing in stop.
So this is removed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] szetszwo commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r992084368
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/http/RatisDropwizardExports.java:
##########
@@ -54,6 +54,13 @@ public static void registerRatisMetricReporters(
r2 -> deregisterDropwizard(r2, ratisMetricsMap));
}
+ public static void clear(Map<String, RatisDropwizardExports>
+ ratisMetricsMap) {
+ MetricRegistries.global().clear();
+ CollectorRegistry.defaultRegistry.clear();
+ ratisMetricsMap.clear();
+ }
Review Comment:
+1 the change looks good.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r984182546
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/http/RatisDropwizardExports.java:
##########
@@ -54,6 +54,13 @@ public static void registerRatisMetricReporters(
r2 -> deregisterDropwizard(r2, ratisMetricsMap));
}
+ public static void clear(Map<String, RatisDropwizardExports>
+ ratisMetricsMap) {
+ MetricRegistries.global().clear();
+ CollectorRegistry.defaultRegistry.clear();
+ ratisMetricsMap.clear();
+ }
Review Comment:
Hey @szetszwo, could you help to review this Ratis metrics un-register logic ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
sumitagrawl commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r991196670
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMBlockDeletingService.java:
##########
@@ -229,7 +229,7 @@ public String getServiceName() {
@Override
public void stop() {
- throw new RuntimeException("Not supported operation.");
+ // Not supported operation
Review Comment:
I checked, BlockManagerImpl is also calling close of blockingService twice (may be code bug but not in scope). But this has no impact.
Considering this, will call shutdown() in stop() of SCMBlockDeletingService, and this do not have any impact for code reuse.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3787:
URL: https://github.com/apache/ozone/pull/3787#issuecomment-1274862517
@ChenSammi please wait for clean CI before merging PRs. It seems two integration tests are timing out due to this change:
```
org.apache.hadoop.ozone.dn.ratis.TestDnRatisLogParser
org.apache.hadoop.ozone.container.common.statemachine.commandhandler.TestBlockDeletion
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
sumitagrawl commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r990904312
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMBlockDeletingService.java:
##########
@@ -229,7 +229,7 @@ public String getServiceName() {
@Override
public void stop() {
- throw new RuntimeException("Not supported operation.");
+ // Not supported operation
Review Comment:
1. For this PR, will avoid serviceManager.stop() and scope out this cleanup
2. OR remove serviceManager.stop() and add explicit member fields of class and do cleanup as SCM stop
3. OR SCMBlockDeletingService stop() call internally shutdown()
Which process should we do currently?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
sumitagrawl commented on PR #3787:
URL: https://github.com/apache/ozone/pull/3787#issuecomment-1261976036
@ChenSammi @nandakumar131 Please review
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3787:
URL: https://github.com/apache/ozone/pull/3787#issuecomment-1274998145
I have reverted the change, please fix the two failing tests and open new PR.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r984188722
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMBlockDeletingService.java:
##########
@@ -229,7 +229,7 @@ public String getServiceName() {
@Override
public void stop() {
- throw new RuntimeException("Not supported operation.");
+ // Not supported operation
Review Comment:
Please keep the origin implementation. It's the common practice for unsupported operation.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
sumitagrawl commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r985353464
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/BackgroundSCMService.java:
##########
@@ -126,7 +126,6 @@ public void stop() {
log.info("{} Service is not running, skip stop.", getServiceName());
return;
}
- backgroundThread.interrupt();
Review Comment:
This is fixed
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
ChenSammi merged PR #3787:
URL: https://github.com/apache/ozone/pull/3787
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r984188722
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMBlockDeletingService.java:
##########
@@ -229,7 +229,7 @@ public String getServiceName() {
@Override
public void stop() {
- throw new RuntimeException("Not supported operation.");
+ // Not supported operation
Review Comment:
Please keep the origin implementation.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #3787: HDDS-7271. Ozone Integration test shows memory leak (graceful shutdown cleanup)
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #3787:
URL: https://github.com/apache/ozone/pull/3787#discussion_r984190767
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/BackgroundSCMService.java:
##########
@@ -126,7 +126,6 @@ public void stop() {
log.info("{} Service is not running, skip stop.", getServiceName());
return;
}
- backgroundThread.interrupt();
Review Comment:
Why remove this 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org