ctubbsii commented on code in PR #33:
URL:
https://github.com/apache/accumulo-classloaders/pull/33#discussion_r2695482623
##########
modules/local-caching-classloader/pom.xml:
##########
@@ -131,6 +147,26 @@
</dependencies>
<build>
<plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration>
+ <showDeprecation>true</showDeprecation>
+ <showWarnings>true</showWarnings>
+ <compilerArgs>
+ <arg>-Xlint:all</arg>
+ <arg>-Xlint:-processing</arg>
+ <arg>-Xmaxwarns</arg>
+ <arg>5</arg>
+ </compilerArgs>
Review Comment:
You can omit these bits, as they are inherited from the parent POM.
```suggestion
```
In fact, none of these changes to the compiler plugin execution are strictly
needed. Another way to do this without modifying the plugin configuration is to
just add auto-service as a module dependency with the
`<optional>true</optional>`.
##########
modules/local-caching-classloader/pom.xml:
##########
@@ -51,6 +51,16 @@
<artifactId>spotbugs-annotations</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>com.google.auto.service</groupId>
+ <artifactId>auto-service-annotations</artifactId>
+ <version>1.1.1</version>
Review Comment:
If you don't set the annotation processor paths in the compiler plugin, this
will get automatically picked up by the compiler, since it's a dependency on
the classpath and contains an annotation processor. It can be marked
`<optional>true</optional>`, since it's not actually needed after compilation,
and other projects who might add this as a dependency shouldn't pick this up
transitively. `<optional>true</optional>` is a way of excluding it transitively.
The version for this, however, should be managed in the parent POM (top
level of this project), in a dependencyManagement section.
##########
modules/local-caching-classloader/src/test/java/org/apache/accumulo/classloader/lcc/MiniAccumuloClusterClassLoaderFactoryTest.java:
##########
@@ -268,6 +319,40 @@ public void testClassLoader() throws Exception {
}
}
+ public Set<String> getReferencedFiles() {
+ final Map<String,List<String>> referencedFiles = new HashMap<>();
+ List<VirtualMachineDescriptor> vmdl = VirtualMachine.list();
+ for (VirtualMachineDescriptor vmd : vmdl) {
Review Comment:
```suggestion
for (VirtualMachineDescriptor vmd : VirtualMachine.list()) {
```
##########
modules/local-caching-classloader/src/test/java/org/apache/accumulo/classloader/lcc/MiniAccumuloClusterClassLoaderFactoryTest.java:
##########
@@ -268,6 +319,40 @@ public void testClassLoader() throws Exception {
}
}
+ public Set<String> getReferencedFiles() {
Review Comment:
This can be private. Several of the variables lines in this method could use
`var` to make the code shorter and more readable.
For example, `proxy`, `url`, and `vm`.
##########
pom.xml:
##########
@@ -419,7 +419,7 @@ under the License.
<reactorModuleConvergence />
<banDuplicatePomDependencyVersions />
<dependencyConvergence />
- <banDynamicVersions />
+ <!-- <banDynamicVersions />-->
Review Comment:
```suggestion
<banDynamicVersions>
<ignores>
<!-- allow Accumulo snapshots -->
<ignore>org.apache.accumulo:*:*-SNAPSHOT</ignore>
</ignores>
</banDynamicVersions>
```
##########
modules/local-caching-classloader/src/test/java/org/apache/accumulo/classloader/lcc/MiniAccumuloClusterClassLoaderFactoryTest.java:
##########
@@ -268,6 +319,40 @@ public void testClassLoader() throws Exception {
}
}
+ public Set<String> getReferencedFiles() {
+ final Map<String,List<String>> referencedFiles = new HashMap<>();
+ List<VirtualMachineDescriptor> vmdl = VirtualMachine.list();
+ for (VirtualMachineDescriptor vmd : vmdl) {
+ if (vmd.displayName().contains("org.apache.accumulo.start.Main")
+ && !vmd.displayName().contains("zookeeper")) {
+ LOG.info("Attempting to connect to {}", vmd.displayName());
+ try {
+ VirtualMachine vm = VirtualMachine.attach(vmd);
+ String connectorAddress = vm.getAgentProperties()
+
.getProperty("com.sun.management.jmxremote.localConnectorAddress");
+ if (connectorAddress == null) {
+ connectorAddress = vm.startLocalManagementAgent();
+ connectorAddress = vm.getAgentProperties()
+
.getProperty("com.sun.management.jmxremote.localConnectorAddress");
+ }
+ JMXServiceURL url = new JMXServiceURL(connectorAddress);
+ try (JMXConnector connector = JMXConnectorFactory.connect(url)) {
+ MBeanServerConnection mbsc = connector.getMBeanServerConnection();
+ ContextClassLoadersMXBean proxy = JMX.newMXBeanProxy(mbsc,
+ ContextClassLoadersMXBean.getObjectName(),
ContextClassLoadersMXBean.class);
+ referencedFiles.putAll(proxy.getReferencedFiles());
+ }
+ } catch (Exception e) {
Review Comment:
This is just test code, so it isn't really that important, but in general, I
think these overly-broad exception handling is bad practice. The reason for
this is that if new code introduces a new exception, the developer may not
notice and may not realize if it needs to be handled differently than the
existing catch block. It may be okay to handle it the same as the existing
catch block, but it's better to force the developer to triage that, and make
that decision, rather than have the new Exception automatically swallowed up.
Since the README references this as an example, this should be avoided. It's
better to enumerate the checked exceptions in a multi-catch block. If you want
to catch RuntimeExceptions as well, you can add that into the same multi-catch
block with the checked exceptions.
Also, this test may want to throw the exception, rather than ignore it after
logging, so it causes the test to fail.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/DeduplicationCache.java:
##########
@@ -75,4 +76,9 @@ public boolean anyMatch(final Predicate<KEY> keyPredicate) {
return
canonicalWeakValuesCache.asMap().keySet().stream().anyMatch(keyPredicate);
}
+ public void values(final BiConsumer<KEY,VALUE> consumer) {
Review Comment:
This isn't just the values
```suggestion
public void forEach(final BiConsumer<KEY,VALUE> consumer) {
```
##########
modules/local-caching-classloader/src/test/java/org/apache/accumulo/classloader/lcc/MiniAccumuloClusterClassLoaderFactoryTest.java:
##########
@@ -130,9 +152,9 @@ public static void afterAll() throws Exception {
@Test
public void testClassLoader() throws Exception {
-
- var baseDirPath = tempDir.resolve("base");
- var jsonDirPath = baseDirPath.resolve("contextFiles");
+ final var baseDirPath = tempDir.resolve("base");
+ final var resourcesDirPath = baseDirPath.resolve("resources");
+ final var jsonDirPath = baseDirPath.resolve("contextFiles");
Review Comment:
This should be:
```suggestion
final var jsonDirPath = tempDir.resolve("contextFiles");
```
The baseDirPath is the local store used by the classloader. This directory
is for the "remote" context URLs, and can go in a separate directory in the
tempdir.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/LocalCachingContextClassLoaderFactory.java:
##########
@@ -279,4 +300,18 @@ private void checkMonitoredLocation(String
contextLocation, long interval) {
}
+ public static Map<String,List<String>> getReferencedFiles() {
+ final Map<String,List<String>> referencedContexts = new HashMap<>();
+ LocalCachingContextClassLoaderFactory.classloaders.values((defUrl, cl) -> {
Review Comment:
The cachekey contains the source URL, but it is not the same as the source
URL. The more useful part is the checksum, because you can use that to look at
the corresponding contexts/checksum.json file in the local cache.
```suggestion
LocalCachingContextClassLoaderFactory.classloaders.values((cacheKey, cl)
-> {
```
--
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]
