9 Replies Latest reply on Jun 9, 2009 7:11 AM by jaikiran

    MC picking up irrelevant deployers

    jaikiran

      Consider this deployer:

      public class MyDeployer extends AbstractVFSRealDeployer
      {
      
       public MyDeployer()
       {
       addInput("MyRequiredInput");
       }
      
       @Override
       public void deploy(VFSDeploymentUnit unit) throws DeploymentException
       {
       // do some expensive thing here...
       }
      ...
      


      This MyDeployer expects to be called when a deployment unit has an attachment named "MyRequiredInput". Let's assume that adding the attachment with this name to the unit is the responsibility of some ABCDeployer. Based on this, i would expect that if ABCDeployer does *not* add the "MyRequiredInput" attachment to the unit or the ABCDeployer is absent in the runtime environment, then the MyDeployer (which internally does expensive stuff) should *never* be called.


      However, based on what i see currently this deployer gets called for most of the deployments even when the input attachment is not present in the unit. Effectively this deployer ends up doing unnecessary expensive stuff.

      Looking at the code in the DeployersImpl (the isRelevant(...) method), i see that this deployer is considered relevant because of the following code:

      protected boolean isRelevant(Deployer deployer, DeploymentUnit unit, boolean isTopLevel, boolean isComponent)
       {
      ...// some code removed intentionally
      
       if (deployer.isAllInputs() == false)
       {
       // No attachment for the input type
       Class<?> input = deployer.getInput();
       if (input != null && unit.getAttachment(input) == null)
       return false;
       }
       return true;
       }
      


      deployer.getInput() returns null and ultimately this method returns true - marking this deployer as relevant, even though it is not.


       public Class<?> getInput()
       {
       return input;
       }
      

      When an addInput is invoked by a deployer i see this in AbstractDeployer:

       public void addInput(String input)
       {
       if (input == null)
       throw new IllegalArgumentException("Null input");
      
       if (inputs == null)
       inputs = new HashSet<String>();
      
       inputs.add(input);
       }
      


      Nowhere in this call to addInput, does the "input" get set, unlike in setInput:

       public void setInput(Class<?> input)
       {
       addInput(input);
       this.input = input;
       }
      


      Questions:

      1) What's the difference between a addInput and setInput? I had assumed that addInput would add it to an existing set of inputs whereas setInput would overwrite the entire set with a new set. Am i wrong (based on the current behaviour, i think i am wrong)

      2) Shouldn't the deployer be marked as irrelevant unless all its required inputs are available?

      3) Note that this issue applies to almost all deployers which rely on addInput - which means that those deployers too might be doing unnecessary expensive stuff which can be avoided.

      P.S: The MyDeployer in this example is actually a simplified version of EJB3Deployer. And one of the other deployers which seems to be running into this issue is MergedJBossMetaDataDeployer.



        • 1. Re: MC picking up irrelevant deployers
          emuckenhuber

          Yes, setInput() marks the Input as required and addInput() is just used for ordering.

          The Merged*MetaDataDeployers are a special case anyway. It seems they are even replacing the original metadata with the merged result - So other deployers could just require this as input e.g. JBossMetaData. Like the AbstractWarDeployer is doing for the JBossWebMetaData.

          • 2. Re: MC picking up irrelevant deployers
            alesj

            isRelevant() is just a simple check, as you can see it only understands a single input.
            But single input (explicit attachment) is what's mostly used in real deployers.

            It's up to you, deployer dev, to use the appropriate deployer helper.
            In the case of multiple inputs, and we've already covered this use case once I believe, you should use AbstractAllInputDeployer.

            Or it's up to you to short circuit the deploy method/phase.

            • 3. Re: MC picking up irrelevant deployers
              jaikiran

               

              "emuckenhuber" wrote:
              Yes, setInput() marks the Input as required and addInput() is just used for ordering.


              So this raises the next question - what is setInputs() for? I believe for marking multiple inputs as "required". For example ABCDeployer requires InputA *and* InputB for it to be considered relevant:

              public ABCDeployer()
              {
               setInputs("InputA","InputB");
              
              ...
              }
              

              and internally AbstractDeployer.setInputs() does this:
              public void setInputs(Set<String> inputs)
               {
               this.inputs = inputs;
               }


              So going by the code of isRelevant(...) that i posted earlier, this deployer will be considered relevant even when none of InputA or InputB are available.


              As for the EJB3Deployer i tried as you suggested:

              public Ejb3Deployer()
               {
               setInput(JBossMetaData.class); // additional constraint
               addInput(MergedJBossMetaDataDeployer.EJB_MERGED_ATTACHMENT_NAME);
              


              and it works (i.e. the irrelevant deployments are no longer picked up). But i have to do bit more testing to ensure that this deployer is picked at the right time/order (i.e. after the MergedJBossMetadataDeployer is run for the deployment unit)

              • 4. Re: MC picking up irrelevant deployers
                jaikiran

                 

                "alesj" wrote:


                In the case of multiple inputs, and we've already covered this use case once I believe, you should use AbstractAllInputDeployer.

                That's correct, we did discuss some time back about multiple inputs for a deployer and the deployer ordering :) But this case is different because EJB3Deployer just relies on a single input and not multiple inputs




                • 5. Re: MC picking up irrelevant deployers
                  alesj

                   

                  "jaikiran" wrote:
                  But this case is different because EJB3Deployer just relies on a single input and not multiple inputs

                  If that's just a String key, that's a special use case.
                  Simply short circuit it on the first line of deploy(),
                  and all you would gain is one more invocation, which almost shouldn't show.

                  • 6. Re: MC picking up irrelevant deployers
                    jaikiran

                     

                    "alesj" wrote:

                    Simply short circuit it on the first line of deploy(),
                    and all you would gain is one more invocation, which almost shouldn't show.


                    I think we will instead change the EJB3Deployer to extend from AbstractSimpleVFSRealDeployer and let the addInput(MergedJBossMetaDataDeployer.EJB_MERGED_ATTACHMENT_NAME); remain to allow proper ordering:


                    public class Ejb3Deployer extends AbstractSimpleVFSRealDeployer<JBossMetaData>
                    {
                    
                     public Ejb3Deployer()
                     {
                     addInput(MergedJBossMetaDataDeployer.EJB_MERGED_ATTACHMENT_NAME);
                    


                    "alesj" wrote:

                    If that's just a String key, that's a special use case.


                    I didn't get this. You mean deployers aren't expected (in general) to have a String key as a requirement?




                    • 7. Re: MC picking up irrelevant deployers
                      alesj

                       

                      "jaikiran" wrote:

                      I didn't get this. You mean deployers aren't expected (in general) to have a String key as a requirement?

                      Sure they are, but not like you did, a single input string.
                      You had addInput("somestring"), where the usual use case is setInput(Class attachmentsClass).
                      Hence isRelevant cannot work in your case.

                      • 8. Re: MC picking up irrelevant deployers
                        jaikiran

                         

                        where the usual use case is setInput(Class attachmentsClass).

                        That would be adding a requirement on a Class and not a string, isn't it? Looking at the API available, i think there's no way to have a deployer that requires just a single String key and expect the deployer to be invoked only when the string key is available as attachment.




                        • 9. Re: MC picking up irrelevant deployers
                          jaikiran

                           

                          "jaikiran" wrote:

                          I think we will instead change the EJB3Deployer to extend from AbstractSimpleVFSRealDeployer<JBossMetaData> and let the addInput(MergedJBossMetaDataDeployer.EJB_MERGED_ATTACHMENT_NAME); remain to allow proper ordering


                          https://jira.jboss.org/jira/browse/JBAS-7004