Re: [PR] Shutdown expunged resources cleanup executor properly, and allow other components to configure/start/stop on error [cloudstack]

[via GitHub]

JoaoJandre commented on code in PR #9723:
URL: https://github.com/apache/cloudstack/pull/9723#discussion_r1771763118


##########
framework/spring/lifecycle/src/main/java/org/apache/cloudstack/spring/lifecycle/CloudStackExtendedLifeCycle.java:
##########
@@ -93,13 +98,21 @@ public void with(ComponentLifecycle lifecycle) {
     }
 
     public void stopBeans() {
+        logger.info("Stopping CloudStack Components");
+
         with(new WithComponentLifeCycle() {
             @Override
             public void with(ComponentLifecycle lifecycle) {
                 logger.info("stopping bean " + lifecycle.getName());
-                lifecycle.stop();
+                try {
+                    lifecycle.stop();
+                } catch (Exception e) {
+                    logger.error(String.format("Error on stopping bean %s - 
%s", lifecycle.getName(), e.getMessage()), e);

Review Comment:
   ```suggestion
                       logger.error("Error on stopping bean {} - {}", 
lifecycle.getName(), e.getMessage(), e);
   ```



##########
framework/spring/lifecycle/src/main/java/org/apache/cloudstack/spring/lifecycle/CloudStackExtendedLifeCycle.java:
##########
@@ -109,10 +122,13 @@ private void configure() {
             @Override
             public void with(ComponentLifecycle lifecycle) {
                 try {
+                    logger.info("configuring bean " + lifecycle.getName());
                     lifecycle.configure(lifecycle.getName(), 
lifecycle.getConfigParams());
                 } catch (ConfigurationException e) {
                     logger.error("Failed to configure " +  
lifecycle.getName(), e);
                     throw new CloudRuntimeException(e);
+                } catch (Exception e) {
+                    logger.error(String.format("Error on configuring bean %s - 
%s", lifecycle.getName(), e.getMessage()), e);

Review Comment:
   ```suggestion
                       logger.error("Error on configuring bean {} - {}", 
lifecycle.getName(), e.getMessage(), e);
   ```



##########
framework/spring/lifecycle/src/main/java/org/apache/cloudstack/spring/lifecycle/CloudStackExtendedLifeCycle.java:
##########
@@ -69,7 +69,12 @@ public void startBeans() {
         with(new WithComponentLifeCycle() {
             @Override
             public void with(ComponentLifecycle lifecycle) {
-                lifecycle.start();
+                logger.info("starting bean " + lifecycle.getName());
+                try {
+                    lifecycle.start();
+                } catch (Exception e) {
+                    logger.error(String.format("Error on starting bean %s - 
%s", lifecycle.getName(), e.getMessage()), e);

Review Comment:
   ```suggestion
                       logger.error("Error on starting bean {} - {}", 
lifecycle.getName(), e.getMessage(), e);
   ```



##########
framework/spring/lifecycle/src/main/java/org/apache/cloudstack/spring/lifecycle/CloudStackExtendedLifeCycle.java:
##########
@@ -109,10 +122,13 @@ private void configure() {
             @Override
             public void with(ComponentLifecycle lifecycle) {
                 try {
+                    logger.info("configuring bean " + lifecycle.getName());

Review Comment:
   ```suggestion
                       logger.info("configuring bean {}.", lifecycle.getName());
   ```



##########
framework/spring/lifecycle/src/main/java/org/apache/cloudstack/spring/lifecycle/CloudStackExtendedLifeCycle.java:
##########
@@ -69,7 +69,12 @@ public void startBeans() {
         with(new WithComponentLifeCycle() {
             @Override
             public void with(ComponentLifecycle lifecycle) {
-                lifecycle.start();
+                logger.info("starting bean " + lifecycle.getName());

Review Comment:
   ```suggestion
                   logger.info("starting bean {}.", lifecycle.getName());
   ```



##########
framework/spring/module/src/main/java/org/apache/cloudstack/spring/module/factory/CloudStackSpringContext.java:
##########
@@ -77,8 +77,10 @@ public void registerShutdownHook() {
         for (String appName : contextMap.keySet()) {
             ApplicationContext contex = contextMap.get(appName);
             if (contex instanceof ConfigurableApplicationContext) {
-                logger.trace("registering shutdown hook for bean "+ appName);
+                logger.trace("Registering shutdown hook for bean "+ appName);
                 
((ConfigurableApplicationContext)contex).registerShutdownHook();
+            } else {
+                logger.warn("Shutdown hook not registered for bean " + 
appName);

Review Comment:
   ```suggestion
                   logger.warn("Shutdown hook not registered for bean {}", 
appName);
   ```



##########
framework/spring/module/src/main/java/org/apache/cloudstack/spring/module/factory/CloudStackSpringContext.java:
##########
@@ -77,8 +77,10 @@ public void registerShutdownHook() {
         for (String appName : contextMap.keySet()) {
             ApplicationContext contex = contextMap.get(appName);
             if (contex instanceof ConfigurableApplicationContext) {
-                logger.trace("registering shutdown hook for bean "+ appName);
+                logger.trace("Registering shutdown hook for bean "+ appName);

Review Comment:
   ```suggestion
                   logger.trace("Registering shutdown hook for bean {}.", 
appName);
   ```



-- 
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: commits-unsubscr...@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org