Skip to content

Conversation

@s12v
Copy link
Contributor

@s12v s12v commented May 30, 2017

Currently, entire classpath is in exception message, which is not very helpful:

Caused by: java.lang.IllegalStateException: jar hell!
duplicate jar on classpath: /Applications/IntelliJ IDEA.app/Contents/lib/...

After the change, only problematic jar should be included in the message.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@jasontedor
Copy link
Member

I see the point about making it easier to see the duplicate jar, but I would prefer that we not suppress the entire classpath here. Would you mind pushing a change the shows the duplicate jar and the entire classpath? Something like:

duplicate jar [/path/to/jar.jar] on classpath: 

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment at the top level about changing the method to still include the entire classpath.

@s12v
Copy link
Contributor Author

s12v commented May 30, 2017

@jasontedor, updated

@jasontedor
Copy link
Member

test this please

@jasontedor
Copy link
Member

Thanks @s12v, I've started a CI build.

* master: (62 commits)
  Handle already closed while filling gaps
  [DOCS] Clarify behaviour of scripted-metric arg with empty parent buckets
  [DOCS] Clarify connections and gateway nodes selection in cross cluster search docs (elastic#24859)
  Java api: Remove unneeded getTookInMillis method (elastic#23923)
  Adds nodes usage API to monitor usages of actions (elastic#24169)
  Add superset size to Significant Term REST response (elastic#24865)
  Disallow multiple parent-join fields per mapping (elastic#25002)
  [Test] Remove unused test resources in core (elastic#25011)
  Scripting: Add optional context parameter to put stored script requests (elastic#25014)
  Extract a common base class for scroll executions (elastic#24979)
  Build: fix version sorting
  Build: Move verifyVersions to new branchConsistency task (elastic#25009)
  Add backwards compatibility indices
  Build: improve verifyVersions error message (elastic#25006)
  Add version 5.4.2 constant
  Docs: More search speed advices. (elastic#24802)
  Add version 5.3.3 constant
  Reorganize docs of global ordinals. (elastic#24982)
  Provide the TransportRequest during validation of a search context (elastic#24985)
  [TEST] fix SearchIT assertion to also accept took set to 0
  ...
@jasontedor
Copy link
Member

retest this please

@s12v
Copy link
Contributor Author

s12v commented Jun 2, 2017

@jasontedor, there's a typo, sorry. Fixed, could you please retest it?

@jasontedor
Copy link
Member

retest this please

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jasontedor jasontedor merged commit 57b4002 into elastic:master Jun 2, 2017
jasontedor pushed a commit that referenced this pull request Jun 2, 2017
When the jarhell check fails due to a duplicate jar on the classpath,
the exception message includes the full classpath but not the duplicated
jar. For a long classpath, this can make it difficult to find the jar
that is duplicated. This commit changes the exception message to include
the duplicated jar.

Relates #24953
@jasontedor
Copy link
Member

Thanks @s12v!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants