gemmellr commented on code in PR #1563:
URL: https://github.com/apache/activemq/pull/1563#discussion_r2768182075
##########
activemq-client/src/main/java/org/apache/activemq/thread/TaskRunnerFactory.java:
##########
@@ -273,13 +267,20 @@ protected TaskRunner
createVirtualThreadTaskRunner(Executor executor, Task task,
if(!TaskRunner.class.isAssignableFrom(result.getClass())) {
throw new IllegalStateException("VirtualThreadTaskRunner not
returned");
}
- return TaskRunner.class.cast(result);
+ return (TaskRunner) result;
} catch (ClassNotFoundException | NoSuchMethodException |
SecurityException | IllegalAccessException | InvocationTargetException |
InstantiationException | IllegalArgumentException e) {
LOG.error("VirtualThreadTaskRunner class failed to load", e);
throw new IllegalStateException(e);
}
}
+ private void assertJDK21VirtualThreadSupport() {
+ if(!(Runtime.version().feature() >= 21)) {
+ LOG.error("Virtual Thread support requires JDK 21 or higher");
+ throw new IllegalStateException("Virtual Thread support requires
JDK 21 or higher");
+ }
+ }
+
Review Comment:
The cleanup seems fine, but a thought for later would be....is there any
need for all the reflection above and this runtime version checking, when the
client jar is already a multi-release jar requiring Java 21 (and soon 24[/25])
to release it? Could maybe have a [couple variants of] trivial delegate class.
##########
activemq-unit-tests/pom.xml:
##########
@@ -465,6 +465,18 @@
<exclude>**/ClientIdFilterDispatchPolicyTest.java</exclude>
<exclude>**/StartAndConcurrentStopBrokerTest.java</exclude>
<exclude>**/BlobTransferPolicyUriTest.java</exclude>
+
+ <!--
+ added when running under Java 21+, but it can't run as part of
normal tests
+ because Maven Surefire will use activemq-client/target/classes
instead of the MRJAR
+
+ This test needs to run as part of the integration test phase to
ensure it's using the packaged version
+ We use failsafe instead which is a better choice for integration
tests. See bellow the configuration.
+ -->
Review Comment:
See earlier comment for potential route to avoid such config as this and the
later inversion of it.
##########
activemq-unit-tests/pom.xml:
##########
@@ -507,6 +519,37 @@
</dependency>
</dependencies>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-failsafe-plugin</artifactId>
+ <version>3.5.2</version>
Review Comment:
If still adding it (rather than omitting from previous suggestion), the
plugin version should be managed rather than specified here, and match the
surefire version (it definitely already is managed by the apache parent, though
I believe activemq might be overriding the surefire version prop somewhat
superflously)
##########
activemq-broker/pom.xml:
##########
@@ -263,5 +263,32 @@
</plugins>
</build>
</profile>
+ <profile>
+ <id>jdk24-plus</id>
+ <activation>
+ <jdk>[24,)</jdk>
+ </activation>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>java24-compile</id>
+ <phase>compile</phase>
+ <goals>
+ <goal>compile</goal>
+ </goals>
+ <configuration>
+ <release>24</release> <!-- Specific Java version for
alternative classes -->
+
<compileSourceRoots>${project.basedir}/src/main/java24</compileSourceRoots>
+ <multiReleaseOutput>true</multiReleaseOutput>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
Review Comment:
Elsewhere after the compile defintion I also just brought forward the jar
preparation in the multi-release module, to before the test phase rather than
after, thus making the jar and multi-release classes in it available to the
other modules tests.
```
<plugin>
<artifactId>maven-jar-plugin</artifactId>
<!-- Do jar creation earlier, before the "test" phase, so that the
multi-release-jar content is visible to any dependent modules,
enabling them to use the relevant classes during their tests. -->
<executions>
<execution>
<id>default-jar</id>
<phase>process-test-classes</phase>
<goals>
<goal>jar</goal>
</goals>
</execution>
</executions>
</plugin>
```
##########
activemq-broker/src/main/java24/org/apache/activemq/broker/jmx/SubjectShim.java:
##########
@@ -0,0 +1,35 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.broker.jmx;
+
+import javax.security.auth.Subject;
+import java.security.AccessController;
Review Comment:
Looks like you have some stale commits from another PR.
Same months-old feedback [as on the other
PR](https://github.com/apache/activemq/pull/1533#discussion_r2533507877), this
is unused and shouldnt be here (and will break things when the SecurityManager
APIs are eventually removed)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
