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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
