[GitHub] [metron] mmiklavc commented on issue #1436: METRON-2149: Shaded jar classifier is not consistent

[GitBox <gi...@apache.org>]

mmiklavc commented on issue #1436: METRON-2149: Shaded jar classifier is not consistent
URL: https://github.com/apache/metron/pull/1436#issuecomment-507844335
 
 
   > To get to a point where we can satisfy all 3 requirements above, I can think of a couple options. Both of these options are based on the assumption that most class version conflicts involve Stellar classes. Stellar contains most of our business logic and contains a long list of dependencies, including several that commonly conflict with other projects (guava, log4j, jackson, etc). The idea behind both of these options involves isolating Stellar from the rest of the project. Here they are:
   > 
   > 1. Make Stellar the exception and remove the classifier on stellar-common.  This module would be the only one that does this.  This satisfies 3 as long as code requiring different class versions is located in this module.  This means we may need to move classes into this module (or do this with other modules too).  To satisfy 1 and 2 we would need to ensure we are rewriting ALL transitive dependencies or tolerate relocating classes as we run into issues.  The advantage of this approach is there would still be a single uber jar so changes to scripts and classpath setup would not change.  The disadvantage is there is still the risk of transitive dependencies leaking into the main uber jar.
   > 2. Deploy Stellar code in a separate jar and add it to classpath after the main uber jar, whatever that is (metron-data-management, metron-enrichment-storm, etc).  This satisfies 3 because the separate Stellar jar can contain the relocated classes but other dependencies will not overwrite dependencies of the main uber jar (because it's listed after the main uber jar).  1 and 2 are not a concern when classifiers are used which is the case here.  The main disadvantage I see is that there will be work adding this extra jar in the various scripts or startup options and we may have to reorganize some classes.
   
   > This is all fairly complex so if anything is not clear I can elaborate. Are there other options I'm not thinking of? Thoughts?
   
   I really like your 3 proposed main requirements - those sound reasonable to me, with varying degrees of importance depending on how you look at them. I'd say that 3 is the most important in practice, with 1 and 2 supporting that goal.
   
   First big question I have is what about Stellar functions in metron-analytics? As I pointed out in this [comment](https://github.com/apache/metron/pull/1436#issuecomment-498369511) those are not core Stellar functions, but I suspect there's potential trouble there as well.
   
   **Other thoughts**
   
   Would it make sense to shade and relocate all of the core libs for Stellar and mark all others as scope `provided`? The litmus test here might be similar to how this is handled in J2EE web applications. 
   1. If the web container (analogous to Storm in this case) provides a specific library, mark it as provided. 
   2. If we absolutely want/need a conflicting version, then we would shade/relocate it. e.g. `com.fasterxml.jackson` is relocated as `org.apache.metron.jackson.${metron_jackson_version}` (credit @nickwallen on his work in the profiler libs with Guava relocation)
   
   The catch with still using an uber jar (ie dep with the "uber" jar classifier) is that modules depending on it would still need to exclude the transitive dependencies that were relocated. The short of it being that the uber jar would have the relocated packages (e.g. org.apache.metron.jackson), but the transitive dependency still carries through as com.fasterxml.jackson when our uber jar is referenced as a dep. This is effectively very very similar to what you outlined and solved in approach 2 with the separate uber jar deploy, but should also cover the broader project classpath for all cases.
   
   We spent multiple hours working on this together offline, so I won't rehash all of that here - you've done a reasonable job of summarizing a lot of the main bits. Thanks @merrimanr!

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services