[GitHub] [felix-dev] JochenHiller commented on a diff in pull request #192: Updated to use HTTP optionally in HealthCheck

["JochenHiller (via GitHub)" <gi...@apache.org>]

JochenHiller commented on code in PR #192:
URL: https://github.com/apache/felix-dev/pull/192#discussion_r1089011866


##########
healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/servlet/HealthCheckExecutorServlet.java:
##########
@@ -162,7 +164,10 @@ void setDescription(String d) {
     ResultTxtVerboseSerializer verboseTxtSerializer;
 
     @Activate
-    protected final void activate(final HealthCheckExecutorServletConfiguration configuration) {
+    protected final void activate(final HealthCheckExecutorServletConfiguration configuration, final BundleContext bundleContext) {
+    	this.bundleContext = bundleContext;
+    	servletRegistrations = new ArrayList<>();

Review Comment:
   Assign with `this.servletRegistrations`  to follow same code style?



##########
healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/servlet/HealthCheckExecutorServlet.java:
##########
@@ -215,29 +219,24 @@ protected final void activate(final HealthCheckExecutorServletConfiguration conf
         for (final Map.Entry<String, HttpServlet> servlet : servletsToRegister.entrySet()) {
             try {
                 LOG.info("Registering HC servlet {} to path {}", getClass().getSimpleName(), servlet.getKey());
-                this.httpService.registerServlet(servlet.getKey(), servlet.getValue(), null, null);
+                registerServlet(servlet.getValue(), servlet.getKey());
             } catch (Exception e) {
                 LOG.error("Could not register health check servlet: " + e, e);
             }
         }
-        this.servletPaths = servletsToRegister.keySet().toArray(new String[0]);
     }
 
     @Deactivate
     public void deactivate() {
-        if (this.servletPaths == null) {
-            return;
-        }
-
-        for (final String servletPath : this.servletPaths) {
-            try {
-                LOG.info("Unregistering HC Servlet {} from path {}", getClass().getSimpleName(), servletPath);
-                this.httpService.unregister(servletPath);
-            } catch (Exception e) {
-                LOG.error("Could not unregister health check servlet: " + e, e);
-            }
-        }
-        this.servletPaths = null;
+        servletRegistrations.forEach(ServiceRegistration::unregister);
+    }
+    
+    private void registerServlet(Servlet servlet, String path) {

Review Comment:
   Better to reverse argument ordering? Then it would match closer the `httpService.registerServlet()`.
   
   E.g.
   
   ```
   private void registerServlet(String path, Servlet servlet) {
   ```



##########
healthcheck/core/src/main/java/org/apache/felix/hc/core/impl/servlet/HealthCheckExecutorServlet.java:
##########
@@ -215,29 +219,24 @@ protected final void activate(final HealthCheckExecutorServletConfiguration conf
         for (final Map.Entry<String, HttpServlet> servlet : servletsToRegister.entrySet()) {
             try {
                 LOG.info("Registering HC servlet {} to path {}", getClass().getSimpleName(), servlet.getKey());
-                this.httpService.registerServlet(servlet.getKey(), servlet.getValue(), null, null);
+                registerServlet(servlet.getValue(), servlet.getKey());
             } catch (Exception e) {
                 LOG.error("Could not register health check servlet: " + e, e);
             }
         }
-        this.servletPaths = servletsToRegister.keySet().toArray(new String[0]);
     }
 
     @Deactivate
     public void deactivate() {
-        if (this.servletPaths == null) {
-            return;
-        }
-
-        for (final String servletPath : this.servletPaths) {
-            try {
-                LOG.info("Unregistering HC Servlet {} from path {}", getClass().getSimpleName(), servletPath);
-                this.httpService.unregister(servletPath);
-            } catch (Exception e) {
-                LOG.error("Could not unregister health check servlet: " + e, e);
-            }
-        }
-        this.servletPaths = null;
+        servletRegistrations.forEach(ServiceRegistration::unregister);

Review Comment:
   Nice code, but the info that HC Servlet has been unregistered is missing compared to previous code. I would propose to have the logs like before so you see what has been registered and unregistered later.
   
   Do you need to remove the entries from `servletRegistrations` or can we rely that no-one is using that variable after component de-activation?



-- 
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: dev-unsubscribe@felix.apache.org

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