You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by "amitjoy (via GitHub)" <gi...@apache.org> on 2023/01/26 11:56:48 UTC
[GitHub] [felix-dev] amitjoy opened a new pull request, #192: Imported OSGi HTTP Package Optionally
amitjoy opened a new pull request, #192:
URL: https://github.com/apache/felix-dev/pull/192
fixes [FELIX-6565](https://issues.apache.org/jira/browse/FELIX-6565)
--
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
[GitHub] [felix-dev] cziegeler merged pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler merged PR #192:
URL: https://github.com/apache/felix-dev/pull/192
--
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
[GitHub] [felix-dev] amitjoy commented on a diff in pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "amitjoy (via GitHub)" <gi...@apache.org>.
amitjoy commented on code in PR #192:
URL: https://github.com/apache/felix-dev/pull/192#discussion_r1089029908
##########
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:
Yeah, agreed 👍
--
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
[GitHub] [felix-dev] amitjoy commented on pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "amitjoy (via GitHub)" <gi...@apache.org>.
amitjoy commented on PR #192:
URL: https://github.com/apache/felix-dev/pull/192#issuecomment-1406536455
@ghenzler @cziegeler Could you kindly have a look at 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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "amitjoy (via GitHub)" <gi...@apache.org>.
amitjoy commented on PR #192:
URL: https://github.com/apache/felix-dev/pull/192#issuecomment-1434437223
@jbonofre if you have time, could you kindly have a look?
--
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
[GitHub] [felix-dev] amitjoy commented on pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "amitjoy (via GitHub)" <gi...@apache.org>.
amitjoy commented on PR #192:
URL: https://github.com/apache/felix-dev/pull/192#issuecomment-1445963609
@jbonofre Could you kindly have a look?
--
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
[GitHub] [felix-dev] amitjoy commented on pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "amitjoy (via GitHub)" <gi...@apache.org>.
amitjoy commented on PR #192:
URL: https://github.com/apache/felix-dev/pull/192#issuecomment-1406537927
@jbonofre Thanks a lot for assigning yourself for the 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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] JochenHiller commented on a diff in pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "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
[GitHub] [felix-dev] JochenHiller commented on pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "JochenHiller (via GitHub)" <gi...@apache.org>.
JochenHiller commented on PR #192:
URL: https://github.com/apache/felix-dev/pull/192#issuecomment-1406626705
Latest changes from @amitjoy LGTM
--
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
[GitHub] [felix-dev] amitjoy commented on pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "amitjoy (via GitHub)" <gi...@apache.org>.
amitjoy commented on PR #192:
URL: https://github.com/apache/felix-dev/pull/192#issuecomment-1406520354
I just added a new commit to update to HTTP whiteboard. There was no runtime issue to use the old pattern of using `HttpService` to register the servlets, but dependency to `HttpService` introduces a mandatory requirement to `osgi.service` namespace.
`osgi.service;filter:="(objectClass=org.osgi.service.http.HttpService)";effective:=active`
This requirement doesn't play any role in the runtime but it plays a signifact role of composing systems. If we use bndtools and use this bundle as an initial requirement to resolve the dependencies, this will fail as the `requirement` needs to be satisfied first. We also use similar mechanism to compose systems using OSGi Resolver which would not work if the comprises such mandatory dependency to `HttpService`. That's why, a cleaner approach would be to go for OSGi HTTP Whiteboard where dependency to `HttpService` is not required at all, whereas, it now optionally requires HTTP whiteboard.
--
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
[GitHub] [felix-dev] amitjoy commented on a diff in pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "amitjoy (via GitHub)" <gi...@apache.org>.
amitjoy commented on code in PR #192:
URL: https://github.com/apache/felix-dev/pull/192#discussion_r1089030958
##########
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:
Yeah, since no one is using it, I didn't remove the entries. I will still clean it up for completeness.
--
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
[GitHub] [felix-dev] amitjoy commented on a diff in pull request #192: Updated to use HTTP optionally in HealthCheck
Posted by "amitjoy (via GitHub)" <gi...@apache.org>.
amitjoy commented on code in PR #192:
URL: https://github.com/apache/felix-dev/pull/192#discussion_r1089029567
##########
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:
Yeah, makes sense. On 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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org