Bug 45485 - Agent.java is sole dependency on jmxtools.jar with Java 5+
Agent.java is sole dependency on jmxtools.jar with Java 5+
Status: RESOLVED FIXED
Product: Log4j
Classification: Unclassified
Component: Other
1.2
PC Windows XP
: P2 normal
: ---
Assigned To: log4j-dev
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2008-07-25 10:30 UTC by Thorbjørn Ravn Andersen
Modified: 2008-08-12 08:45 UTC (History)
0 users



Attachments
Agent.java rewritten with reflection (1.53 KB, patch)
2008-07-25 10:30 UTC, Thorbjørn Ravn Andersen
Details | Diff
Whole file (if patch is bad) (2.08 KB, text/plain)
2008-07-31 09:13 UTC, Thorbjørn Ravn Andersen
Details
Alternative patch that preserves existing log4j behavior (6.61 KB, patch)
2008-08-05 12:15 UTC, Curt Arnold
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thorbjørn Ravn Andersen 2008-07-25 10:30:14 UTC
Created attachment 22319 [details]
Agent.java rewritten with reflection

As part of an effort to remove the need to download components manually (outside Maven) I have found that when compiling with Java 5+ it is only Agent.java which need the jmxtools.jar in the compilation class path.

The attached patch is a rewrite of Agent.java using reflection so it can compile without the presence of jmxtools.jar.  I would appreciate help in getting a test environment running.
Comment 1 Thorbjørn Ravn Andersen 2008-07-31 09:13:45 UTC
Created attachment 22340 [details]
Whole file (if patch is bad)
Comment 2 Thorbjørn Ravn Andersen 2008-07-31 09:14:59 UTC
Additionally these dependencies can be removed from pom.xml

   <dependency>
      <groupId>com.sun.jdmk</groupId>
      <artifactId>jmxtools</artifactId>
      <version>1.2.1</version>
      <optional>true</optional>
    </dependency>
    <dependency>
      <groupId>com.sun.jmx</groupId>
      <artifactId>jmxri</artifactId>
      <version>1.2.1</version>
      <optional>true</optional>
    </dependency>
Comment 3 Paul Smith 2008-07-31 15:32:33 UTC
While I like the idea of removing the compile time dependency to those evil Sun jars, I'm not so sure swallowing an exception is such a good idea, from a stylistic point I think wrapping that exception and throwing an IllegalStateException would be better.

Lets face it, if the Exception does happen, the use probably wants to know about it (yes there's logging, but is continuing in the face of error a good idea here?)

Still, that removal of the Sun jar dependency is tempting! :)
Comment 4 Thorbjørn Ravn Andersen 2008-07-31 15:43:33 UTC
(In reply to comment #3)
> While I like the idea of removing the compile time dependency to those evil Sun
> jars, I'm not so sure swallowing an exception is such a good idea, from a
> stylistic point I think wrapping that exception and throwing an
> IllegalStateException would be better.
> 
> Lets face it, if the Exception does happen, the use probably wants to know
> about it (yes there's logging, but is continuing in the face of error a good
> idea here?)
> 
> Still, that removal of the Sun jar dependency is tempting! :)

Yes.

From what I understand the try-catch-log.error-return approach is the way it is done elsewhere.
Comment 5 Curt Arnold 2008-08-05 12:15:22 UTC
Created attachment 22389 [details]
Alternative patch that preserves existing log4j behavior

This patch changes the checked exceptions raised by reflection into the equivalent error that would occur with the previous code in the same scenario.
Comment 6 Curt Arnold 2008-08-12 08:45:37 UTC
Committed last patch in rev 682879, modified to throw RuntimeException instead of Errors in rev 682998 and 683009 per discussion on log4j-dev.