You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@karaf.apache.org by "CptBartender (via GitHub)" <gi...@apache.org> on 2026/09/15 20:00:58 UTC
[PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
CptBartender opened a new pull request, #2911:
URL: https://github.com/apache/karaf/pull/2911
- Added type parameters to generics
- Changed the class so that it explicitly implements ManagedService instead of sneakily registering itself as such Extracted the string splitting pattern to a static field so that the regex is compiled only once
- Changed getInterfaceNames so that it returns distinct classes in case any implemented interfaces extend the same superinterface
- Changed trackers map to use Class objects as keys instead of String names
- Intermediately bumped required Java version to 17 so that pattern matching for `instanceof` can be used - eventually it'll be bumped in https://github.com/apache/karaf/pull/2214, I presume
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "CptBartender (via GitHub)" <gi...@apache.org>.
CptBartender commented on code in PR #2911:
URL: https://github.com/apache/karaf/pull/2911#discussion_r4172922938
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -320,40 +328,39 @@ public void run() {
* Called in {@link #doOpen()}.
*
* @param clazz The service interface to track.
+ * @param <T> Generic type of the service to track
* @throws InvalidSyntaxException If the tracker syntax is not correct.
*/
- protected void trackService(Class<?> clazz) throws InvalidSyntaxException {
- if (!trackers.containsKey(clazz.getName())) {
- SingleServiceTracker tracker = new SingleServiceTracker<>(bundleContext, clazz, (u, v) -> reconfigure());
- tracker.open();
- trackers.put(clazz.getName(), tracker);
- }
+ protected <T> void trackService(Class<T> clazz) throws InvalidSyntaxException {
+ trackService(clazz, null);
}
/**
* Called in {@link #doOpen()}.
*
* @param clazz The service interface to track.
* @param filter The filter to use to select the services to track.
+ * @param <T> Generic type of the service to track
* @throws InvalidSyntaxException If the tracker syntax is not correct (in the filter especially).
*/
- protected void trackService(Class<?> clazz, String filter) throws InvalidSyntaxException {
- if (!trackers.containsKey(clazz.getName())) {
+ protected <T> void trackService(Class<T> clazz, String filter) throws InvalidSyntaxException {
+ if (!trackers.containsKey(clazz)) {
if (filter != null && filter.isEmpty()) {
filter = null;
}
- SingleServiceTracker tracker = new SingleServiceTracker<>(bundleContext, clazz, filter, (u, v) -> reconfigure());
+ SingleServiceTracker<T> tracker = new SingleServiceTracker<>(bundleContext, clazz, filter, (u, v) -> reconfigure());
tracker.open();
- trackers.put(clazz.getName(), tracker);
+ trackers.put(clazz, tracker);
}
}
protected void trackService(String className, String filter) throws InvalidSyntaxException {
- if (!trackers.containsKey(className)) {
- SingleServiceTracker tracker = new SingleServiceTracker<>(bundleContext, className, filter, (u, v) -> reconfigure());
- tracker.open();
- trackers.put(className, tracker);
- }
+ try {
+ Class<?> clazz = Class.forName(className);
Review Comment:
Reworked
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #2911:
URL: https://github.com/apache/karaf/pull/2911#discussion_r4191778292
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -110,7 +117,9 @@ public void stop(BundleContext context) throws Exception {
scheduled.set(true);
doClose();
executor.shutdown();
- executor.awaitTermination(schedulerStopTimeout, TimeUnit.MILLISECONDS);
+ if (!executor.awaitTermination(schedulerStopTimeout, TimeUnit.MILLISECONDS)) {
+ logger.warn("Executor did not terminate within {} milliseconds", schedulerStopTimeout);
+ }
Review Comment:
This warning also first when the timeout is 0, which is a supported setup. The `feature/core` `Activator` calls `setSchedulerStopTimeout(0)` on purpose, so that `stop()` does not wait for the current job (otherwise the features service could not refresh itself). In that case `awaitTermination(0)` returns `false` as soon as a job is still running, and we log "Executor did not terminate within 0 milliseconds" on a flow that was silent before.
Could you only log when a wait was actually requested?
```suggestion
if (!executor.awaitTermination(schedulerStopTimeout, TimeUnit.MILLISECONDS) && schedulerStopTimeout > 0) {
logger.warn("Executor did not terminate within {} milliseconds", schedulerStopTimeout);
}
```
The suggestion keeps the `awaitTermination` call first, so the behavior for a zero timeout is exactly what it was before the 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: commits-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #2911:
URL: https://github.com/apache/karaf/pull/2911#issuecomment-5969462800
@CptBartender thanks, I'm doing a new pass.
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "CptBartender (via GitHub)" <gi...@apache.org>.
CptBartender commented on code in PR #2911:
URL: https://github.com/apache/karaf/pull/2911#discussion_r4172912509
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -86,8 +96,9 @@ public void start(BundleContext context) throws Exception {
doOpen();
scheduled.set(false);
if (managedServiceRegistration == null
- && trackers.values().stream()
- .allMatch(t -> t.getService() != null)) {
+ && trackers.values().stream()
+ .map(SingleServiceTracker::getService)
+ .allMatch(Objects::nonNull)) {
Review Comment:
Good catch - adding an `&& !trackers.values().isEmpty()` check here.
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #2911:
URL: https://github.com/apache/karaf/pull/2911#discussion_r4027297881
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -364,15 +371,17 @@ protected void trackService(String className, String filter) throws InvalidSynta
* @return The actual tracker service object.
*/
protected <T> T getTrackedService(Class<T> clazz) {
- SingleServiceTracker tracker = trackers.get(clazz.getName());
+ @SuppressWarnings("unchecked")
Review Comment:
The runtime type check on the returned service was dropped. Previously `clazz.cast(tracker.getService())` threw an immediate, clearly-attributed `ClassCastException` if the tracked object wasn't actually assignable to `clazz` (e.g. duplicate class definitions across bundle classloaders). Now it's an unchecked cast on the tracker itself, and `tracker.getService()` is returned with no check: a type mismatch surfaces later (if at all) as a confusing `ClassCastException` far from the real cause, or not at all if passed through untyped code.
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -320,40 +328,39 @@ public void run() {
* Called in {@link #doOpen()}.
*
* @param clazz The service interface to track.
+ * @param <T> Generic type of the service to track
* @throws InvalidSyntaxException If the tracker syntax is not correct.
*/
- protected void trackService(Class<?> clazz) throws InvalidSyntaxException {
- if (!trackers.containsKey(clazz.getName())) {
- SingleServiceTracker tracker = new SingleServiceTracker<>(bundleContext, clazz, (u, v) -> reconfigure());
- tracker.open();
- trackers.put(clazz.getName(), tracker);
- }
+ protected <T> void trackService(Class<T> clazz) throws InvalidSyntaxException {
+ trackService(clazz, null);
}
/**
* Called in {@link #doOpen()}.
*
* @param clazz The service interface to track.
* @param filter The filter to use to select the services to track.
+ * @param <T> Generic type of the service to track
* @throws InvalidSyntaxException If the tracker syntax is not correct (in the filter especially).
*/
- protected void trackService(Class<?> clazz, String filter) throws InvalidSyntaxException {
- if (!trackers.containsKey(clazz.getName())) {
+ protected <T> void trackService(Class<T> clazz, String filter) throws InvalidSyntaxException {
+ if (!trackers.containsKey(clazz)) {
if (filter != null && filter.isEmpty()) {
filter = null;
}
- SingleServiceTracker tracker = new SingleServiceTracker<>(bundleContext, clazz, filter, (u, v) -> reconfigure());
+ SingleServiceTracker<T> tracker = new SingleServiceTracker<>(bundleContext, clazz, filter, (u, v) -> reconfigure());
tracker.open();
- trackers.put(clazz.getName(), tracker);
+ trackers.put(clazz, tracker);
}
}
protected void trackService(String className, String filter) throws InvalidSyntaxException {
- if (!trackers.containsKey(className)) {
- SingleServiceTracker tracker = new SingleServiceTracker<>(bundleContext, className, filter, (u, v) -> reconfigure());
- tracker.open();
- trackers.put(className, tracker);
- }
+ try {
+ Class<?> clazz = Class.forName(className);
Review Comment:
This changes `trackService(String, String)` from pure name-based tracking to eager class resolution via `Class.forName`.
I have two concerns here:
1. This method is `protected`. A subclass calling it with a class name not importable by its own bundle will now get a silently-dropped tracker (just a WARN log) instead of the name-based OSGi filter tracking this overload exists to provide.
2. Only `ClassNotFoundException` is caught. A bad static initializer or a missing transitively-referenced class raises `ExceptionInInitiliazerError`/`NoClassDefFoundError`, which isn't caught here and will propagate out of `doOpen()`/`start(BundleContext)`, aborting the whole bundle's activation. The old code could never throw from this path.
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -86,8 +96,9 @@ public void start(BundleContext context) throws Exception {
doOpen();
scheduled.set(false);
if (managedServiceRegistration == null
- && trackers.values().stream()
- .allMatch(t -> t.getService() != null)) {
+ && trackers.values().stream()
+ .map(SingleServiceTracker::getService)
+ .allMatch(Objects::nonNull)) {
Review Comment:
`allMatch(...)` on `trackers.values()` is vacuously `true` when `trackers` is empty. If a required service's class fails to resolve it `trackService(String, String)`, it's never added to `trackers` at all, so this check now incorrectly reports "ready" instead of waiting.
`doStart()` then calls `getTrackedServices(RequiredInterface.class)`, which throws `IllegalStateException("Service not tracked for class ...)`: a confusing crash disconnected from the real (silently logged) root cause.
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -35,29 +37,37 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.regex.Pattern;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
-
-import org.osgi.framework.*;
+import org.osgi.framework.BundleActivator;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.framework.ServiceReference;
+import org.osgi.framework.ServiceRegistration;
import org.osgi.service.cm.Configuration;
import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ManagedService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class BaseActivator implements BundleActivator, Runnable, ThreadFactory {
+public class BaseActivator implements BundleActivator, ManagedService, Runnable, ThreadFactory {
Review Comment:
Making `BaseActivator` directly `implement ManagedService` turns `org.osgi.service.cm` into a hard compile/link-time dependency for every bundle embedding this class, not just ones that call `manage(pid)`.
The ~ 20 pom.xml additions in this PR cover in-tree callers, but this is a compatibility-breaking change for any downstream/third-party Karaf-based budle that extends `BaseActivator` (including Karaf subprojects like Cellar or Decanter) without Configuration Admin on its classpath: it would now fail to load the class at all (`NoClassDefFoundError`), even if it never calls `manage()`.
I think it's worth calling out explicitly since `BaseActivator` is public API.
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2911:
URL: https://github.com/apache/karaf/pull/2911#issuecomment-5693002153
## Test Results
726 files ±0 726 suites ±0 1h 14m 31s ⏱️ + 2m 35s
985 tests ±0 934 ✅ - 3 48 💤 ±0 0 ❌ ±0 3 🔥 +3
2 955 runs ±0 2 808 ✅ - 3 144 💤 ±0 0 ❌ ±0 3 🔥 +3
For more details on these errors, see [this check](https://github.com/apache/karaf/runs/104688572149).
Results for commit 53200767. ± Comparison against base commit b4831ef9.
[test-results]:data:application/gzip;base64,H4sIANQ1qmoC/03Myw7CIBCF4VdpWLsAOlzqy5hmgITYi6GwMr67g0plQ/J/E86Thbj4g10HI/VlYEeJuUtX0pzjvhEAGEFC11zvk1WtbkdBrDTCn+7xUT/ZU8IcFxJ+gk9pTyQjSSpbHZWTUr9qo9Jye9J3VAA06UY/3W/ivq4xUzA1Ss6NNmCUk9LROwf0xgsvgnOAIXjUE1r2egMq7cUsDwEAAA==
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #2911:
URL: https://github.com/apache/karaf/pull/2911#discussion_r4176149812
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -86,8 +95,10 @@ public void start(BundleContext context) throws Exception {
doOpen();
scheduled.set(false);
if (managedServiceRegistration == null
- && trackers.values().stream()
- .allMatch(t -> t.getService() != null)) {
+ && !trackers.values().isEmpty()
+ && trackers.values().stream()
Review Comment:
My earlier comment here is now out of date, sorry for the churn. It was about the revision where `trackService(String, String)` could drop a tracker when the class failed to resolve. That change has been reverted, so an empty `trackers` map now only means the activator has nothing to wait for.
With this guard, those activators no longer run `doStart()` inside `start()`. They fall through to `reconfigure()` and start later on the executor thread. This affects every activator with no `requires` and no `@Managed`: deployer/blueprint, deployer/spring, docker, instance, package, service/core, system and wrapper. For example, the `blueprint:` URL handler may not be registered yet when the deployer bundle is ACTIVE.
It also means an `Error` thrown by `doStart()` is no longer logged for these activators, because `run()` only catches `Exception`.
Could you remove the guard?
```suggestion
&& trackers.values().stream()
```
##########
pom.xml:
##########
@@ -150,7 +150,9 @@
<properties>
<project.build.outputTimestamp>1695310533</project.build.outputTimestamp>
- <javaVersion>11</javaVersion>
+ <javaVersion>17</javaVersion>
+ <maven.compiler.source>${javaVersion}</maven.compiler.source>
+ <maven.compiler.target>${javaVersion}</maven.compiler.target>
Review Comment:
Bumping `javaVersion` changes the bytecode target of every Karaf module (class version 55 -> 61, `osgi.ee` JavaSE 11 -> 17). That is a baseline change, not a warnings cleanup, and several places still declare Java 11 as supported (I'm working on a PR to bump everything).
For now, I would keep Java 11:
```suggestion
<javaVersion>11</javaVersion>
<maven.compiler.source>${javaVersion}</maven.compiler.source>
<maven.compiler.target>${javaVersion}</maven.compiler.target>
```
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -276,25 +288,24 @@ protected Class<?>[] getClassesArray(String key, String def) {
.toArray(Class[]::new);
}
- protected String[] getStringArray(String key, String def) {
- Object val = null;
+ protected String[] getStringArray(String configKey, String defaultValue) {
+ Object value = null;
if (configuration != null) {
- val = configuration.get(key);
+ value = configuration.get(configKey);
}
- if (val == null) {
- val = def;
+ if (value == null) {
+ value = defaultValue;
}
- if (val == null) {
+ if (value == null) {
return null;
}
- Stream<String> s;
- if (val instanceof String[]) {
- return (String[]) val;
- } else if (val instanceof Iterable) {
- return StreamSupport.stream(((Iterable<?>) val).spliterator(), false)
+ if (value instanceof String[]) {
+ return (String[]) value;
+ } else if (value instanceof Iterable<?> iterableValue) {
+ return StreamSupport.stream(iterableValue.spliterator(), false)
Review Comment:
This pattern match is the only thing in the PR that needs Java 17. Everything else compiles with Java 11. Keeping the cast lets the `pom.xml` changes be dropped entirely.
```suggestion
} else if (value instanceof Iterable) {
return StreamSupport.stream(((Iterable<?>) value).spliterator(), false)
```
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #2911:
URL: https://github.com/apache/karaf/pull/2911#discussion_r4177849428
##########
pom.xml:
##########
@@ -638,7 +640,7 @@
<version>[3.8.8,4)</version>
</requireMavenVersion>
<requireJavaVersion>
- <version>[17,)</version>
+ <version>[${javaVersion},)</version>
Review Comment:
This floor was hard-coded to `[17,)` on purpose in #2231: we need JDK 17+ to build while still targeting Java 11. Trying it to `javaVersion` means that once `javaVersion` is back to 11 the rule becomes `[11,)`, and builds on JDK 11-16 are not longer rejected up front.
Could you restore it (for now, as I have another PR to bump Java version)?
```suggestion
<version>[17,)</version>
```
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -364,19 +367,22 @@ protected void trackService(String className, String filter) throws InvalidSynta
* @return The actual tracker service object.
*/
protected <T> T getTrackedService(Class<T> clazz) {
- SingleServiceTracker tracker = trackers.get(clazz.getName());
+ SingleServiceTracker<?> tracker = trackers.get(clazz.getName());
if (tracker == null) {
throw new IllegalStateException("Service not tracked for class " + clazz);
}
return clazz.cast(tracker.getService());
}
+ @SuppressWarnings("unchecked")
protected <T> ServiceReference<T> getTrackedServiceRef(Class<T> clazz) {
- SingleServiceTracker tracker = trackers.get(clazz.getName());
+ SingleServiceTracker<?> tracker = trackers.get(clazz.getName());
if (tracker == null) {
throw new IllegalStateException("Service not tracked for class " + clazz);
}
- return tracker.getServiceReference();
+ // throw a ClassCastException here if the service is of an invalid type
+ clazz.cast(tracker.getService());
+ return (ServiceReference<T>) tracker.getServiceReference();
Review Comment:
Thanks for restoring the check in `getTrackedService`. I would not mirror it here though.
This method only returns the reference, and the one in-tree caller (the `feature/core` `Activator`) just calls `.getBundle()` on it, so there is no service object whose type needs protecting. The cast adds a `ClassCastException` path that did not exist before. And since the service and the reference are read from two separate `AtomicReference`s, it does not actually validate the reference being returned.
Could you drop it?
```suggestion
```
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -338,19 +344,16 @@ protected void trackService(Class<?> clazz) throws InvalidSyntaxException {
* @throws InvalidSyntaxException If the tracker syntax is not correct (in the filter especially).
*/
protected void trackService(Class<?> clazz, String filter) throws InvalidSyntaxException {
- if (!trackers.containsKey(clazz.getName())) {
- if (filter != null && filter.isEmpty()) {
- filter = null;
- }
- SingleServiceTracker tracker = new SingleServiceTracker<>(bundleContext, clazz, filter, (u, v) -> reconfigure());
- tracker.open();
- trackers.put(clazz.getName(), tracker);
- }
+ trackService(clazz.getName(), filter);
Review Comment:
`BaseActivator` is extended outside the repo, and these three overload are all `protected`. Chaining them changes what a subclass override sees: an override of `trackService(String, String)` now intercepts every tracker, where before it only saw the entries from the `karaf-tracker` properties file.
Nothing in-tree overrides them, so this is not a blocker. But could the shared code move to a private helper instead, so the three overloads stay independent of each other?
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -442,31 +448,32 @@ protected void register(Class[] clazz, Object service) {
* @param service The actual service instance to register.
* @param props The service properties to register.
*/
- protected void register(Class[] clazz, Object service, Dictionary<String, ?> props) {
+ protected void register(Class<?>[] clazz, Object service, Dictionary<String, ?> props) {
String[] names = new String[clazz.length];
for (int i = 0; i < clazz.length; i++) {
names[i] = clazz[i].getName();
}
trackRegistration(bundleContext.registerService(names, service, props));
}
- private void trackRegistration(ServiceRegistration registration) {
+ private void trackRegistration(ServiceRegistration<?> registration) {
registrations.add(registration);
}
protected String[] getInterfaceNames(Object object) {
- List<String> names = new ArrayList<>();
- for (Class cl = object.getClass(); cl != Object.class; cl = cl.getSuperclass()) {
- addSuperInterfaces(names, cl);
+ if (object == null) {
Review Comment:
The previous code failed fast with a `NullPointerException` on `object.getClass()`. With this guard, `registerMBeanWithName(null, name)` goes on to call `registerService(new String[0], null, props)` and fails inside the framework instead, further away from the actual mistake.
I would rather keep the fail-fast behavior. Could you remove the guard?
```suggestion
```
##########
util/src/main/java/org/apache/karaf/util/tracker/SingleServiceTracker.java:
##########
@@ -138,9 +138,7 @@ private boolean update(ServiceReference<T> deadRef, ServiceReference<T> newRef,
Object lock;
// we have to choose our lock.
- if (newRef != null) lock = newRef;
- else if (deadRef != null) lock = deadRef;
- else lock = this;
+ if ((lock = newRef) == null && (lock = deadRef) == null) lock = this;
Review Comment:
This is equivalent to the previous code, but which monitor we take now depends on assignments inside a short-circuit condition, and this is the part of the class where I most want the code to be obvious.
Could you restore the if/else?
```suggestion
if (newRef != null) lock = newRef;
else if (deadRef != null) lock = deadRef;
else lock = this;
```
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -338,19 +344,16 @@ protected void trackService(Class<?> clazz) throws InvalidSyntaxException {
* @throws InvalidSyntaxException If the tracker syntax is not correct (in the filter especially).
*/
protected void trackService(Class<?> clazz, String filter) throws InvalidSyntaxException {
- if (!trackers.containsKey(clazz.getName())) {
- if (filter != null && filter.isEmpty()) {
- filter = null;
- }
- SingleServiceTracker tracker = new SingleServiceTracker<>(bundleContext, clazz, filter, (u, v) -> reconfigure());
- tracker.open();
- trackers.put(clazz.getName(), tracker);
- }
+ trackService(clazz.getName(), filter);
}
protected void trackService(String className, String filter) throws InvalidSyntaxException {
if (!trackers.containsKey(className)) {
- SingleServiceTracker tracker = new SingleServiceTracker<>(bundleContext, className, filter, (u, v) -> reconfigure());
+ if (filter != null && filter.isEmpty()) {
+ filter = null;
+ }
Review Comment:
Nit: the `SingleServiceTracker` constructor already treats `null` and `""` the same way, so this normalization can be removed rather than moved.
```suggestion
```
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -442,31 +448,32 @@ protected void register(Class[] clazz, Object service) {
* @param service The actual service instance to register.
* @param props The service properties to register.
*/
- protected void register(Class[] clazz, Object service, Dictionary<String, ?> props) {
+ protected void register(Class<?>[] clazz, Object service, Dictionary<String, ?> props) {
String[] names = new String[clazz.length];
for (int i = 0; i < clazz.length; i++) {
names[i] = clazz[i].getName();
}
trackRegistration(bundleContext.registerService(names, service, props));
}
- private void trackRegistration(ServiceRegistration registration) {
+ private void trackRegistration(ServiceRegistration<?> registration) {
registrations.add(registration);
}
protected String[] getInterfaceNames(Object object) {
- List<String> names = new ArrayList<>();
- for (Class cl = object.getClass(); cl != Object.class; cl = cl.getSuperclass()) {
- addSuperInterfaces(names, cl);
+ if (object == null) {
+ return new String[0];
}
- return names.toArray(new String[names.size()]);
+ return Stream.<Class<?>>iterate(object.getClass(), not(Object.class::equals), Class::getSuperclass)
+ .flatMap(this::getAllInterfaces)
+ .distinct()
+ .map(Class::getName)
+ .toArray(String[]::new);
Review Comment:
The only functional change here is the de-duplication, and the stream version (`Stream.iterate` plus a recursive `flatMap`) is harder to follow than the loop it replaces. Keeping the loop and collecting into a `LinkedHashSet` gives the same names in the same order:
```java
protected String[] getInterfaceNames(Object object) {
Set<String> names = new LinkedHashSet<>();
for (Class<?> cl = object.getClass(); cl != Object.class; cl = cl.getSuperclass()) {
addSuperInterfaces(names, cl);
}
return names.toArray(new String[0]);
}
private void addSuperInterfaces(Set<String> names, Class<?> clazz) {
for (Class<?> cl : clazz.getInterfaces()) {
names.add(cl.getName());
addSuperInterfaces(names, cl);
}
}
```
The `Arrays` and `Predicate.not` imports added for the stream version can then go.
##########
pom.xml:
##########
@@ -150,7 +150,9 @@
<properties>
<project.build.outputTimestamp>1695310533</project.build.outputTimestamp>
- <javaVersion>11</javaVersion>
+ <javaVersion>17</javaVersion>
+ <maven.compiler.source>${javaVersion}</maven.compiler.source>
+ <maven.compiler.target>${javaVersion}</maven.compiler.target>
Review Comment:
Your current suggestion there keeps the two `maven.compiler.*` lines, this one supersedes it.
Follow-up on my suggestion above: the two `maven.compiler.*` properties can go as well. Apache Parent 40 already sets `maven.compiler.reelease=${javaVersion}` in its `jdk9+` profile, which is always active here since the enforcer requires JDK 17+, and the compiler plugin ignores `source`/`target` when `release` is set.
So this hunk can simple be reverted:
```suggestion
<javaVersion>11</javaVersion>
```
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "CptBartender (via GitHub)" <gi...@apache.org>.
CptBartender commented on code in PR #2911:
URL: https://github.com/apache/karaf/pull/2911#discussion_r4173285127
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -35,29 +37,37 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.regex.Pattern;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
-
-import org.osgi.framework.*;
+import org.osgi.framework.BundleActivator;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.framework.ServiceReference;
+import org.osgi.framework.ServiceRegistration;
import org.osgi.service.cm.Configuration;
import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ManagedService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class BaseActivator implements BundleActivator, Runnable, ThreadFactory {
+public class BaseActivator implements BundleActivator, ManagedService, Runnable, ThreadFactory {
Review Comment:
It seems weird to me that the `BaseActivator` _is_ a `ManagedService` but doesn't implement that interface, but I see your point - reversed.
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -35,29 +37,37 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.regex.Pattern;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
-
-import org.osgi.framework.*;
+import org.osgi.framework.BundleActivator;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.framework.ServiceReference;
+import org.osgi.framework.ServiceRegistration;
import org.osgi.service.cm.Configuration;
import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.cm.ManagedService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class BaseActivator implements BundleActivator, Runnable, ThreadFactory {
+public class BaseActivator implements BundleActivator, ManagedService, Runnable, ThreadFactory {
Review Comment:
It seems weird to me that the `BaseActivator` _is_ a `ManagedService` but doesn't implement that interface, but I see your point - reverted.
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed various IntelliJ warnings in BaseActivator [karaf]
Posted by "CptBartender (via GitHub)" <gi...@apache.org>.
CptBartender commented on code in PR #2911:
URL: https://github.com/apache/karaf/pull/2911#discussion_r4173290239
##########
util/src/main/java/org/apache/karaf/util/tracker/BaseActivator.java:
##########
@@ -364,15 +371,17 @@ protected void trackService(String className, String filter) throws InvalidSynta
* @return The actual tracker service object.
*/
protected <T> T getTrackedService(Class<T> clazz) {
- SingleServiceTracker tracker = trackers.get(clazz.getName());
+ @SuppressWarnings("unchecked")
Review Comment:
Reverted. I also added similar check to the `getTrackedServiceRef`
--
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-unsubscribe@karaf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org