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