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