6 Replies Latest reply on Nov 16, 2007 11:01 AM by Adrian Brock

    MainDeployerImpl.deploy and checkComplete

    Ales Justin Master

      With the full impl of JBMICROCONT-187 (which is done :-)), this needs changing in MainDeployerImpl.deploy:

       public void deploy(Deployment deployment) throws DeploymentException
       {
       addDeployment(deployment);
       process();
       // FIXME don't check contexts
       checkComplete(deployment);
       }
      


      Since it's completely legit to have not yet fully installed contexts - depending beans, OSGi deployments, ...

      Adding 'checkStructureComplete(Deployment)' in DeployerClient --> only checking if deployment is in error or missing deployer?

        • 1. Re: MainDeployerImpl.deploy and checkComplete
          Adrian Brock Master

           

          "alesj" wrote:
          With the full impl of JBMICROCONT-187 (which is done :-)), this needs changing in MainDeployerImpl.deploy:
           public void deploy(Deployment deployment) throws DeploymentException
           {
           addDeployment(deployment);
           process();
           // FIXME don't check contexts
           checkComplete(deployment);
           }
          


          Since it's completely legit to have not yet fully installed contexts - depending beans, OSGi deployments, ...

          Adding 'checkStructureComplete(Deployment)' in DeployerClient --> only checking if deployment is in error or missing deployer?


          No that's not the purpose of the single deployment api.
          It is a short hand way of doing:
          deployerClient.addDeployment(deployment);
          deployerClient.process();
          deployerClient.checkComplete();
          


          Except in a thread safe way. In fact the code should be:
           public void deploy(Deployment deployment) throws DeploymentException
           {
           addDeployment(deployment);
           process(deployment);
           try
           {
           checkComplete(deployment);
           }
           catch (DeploymentException e)
           {
           // Didn't work, roll it back
           removeDeployment(deployment);
           process(deployment);
           throw e;
           }
           }
          


          Where process(deployment) only processes the passed deployment,
          not other deployments that may have been added on other threads.

          But more importantly, it should really be an atomic operation.
          i.e. the deployment is either fully working or it is not deployed at all.

          If you want to add a less strict convience api that is fine,
          but it is a 5% use case not the normal 95% use case of this method.
          It's not related to this issue.

          • 2. Re: MainDeployerImpl.deploy and checkComplete
            Ales Justin Master

            So, MainDeployerImpl.deploy was always meant to be all or nothing?
            Why are then your tests (MockClassLoaderDependenciesUnitTestCase) not relying on this? :-)
            I got this half-check notion from their failure. ;-)

            • 3. Re: MainDeployerImpl.deploy and checkComplete
              Adrian Brock Master

               

              "alesj" wrote:
              So, MainDeployerImpl.deploy was always meant to be all or nothing?
              Why are then your tests (MockClassLoaderDependenciesUnitTestCase) not relying on this? :-)
              I got this half-check notion from their failure. ;-)


              That's the problem with writing tests against a incomplete and not properly implemented api.

              The compiler should have an
              @Incomplete annotation to go with @Deprecated
              to warn you that you are probably writing against an api that will change! ;-)

              • 4. Re: MainDeployerImpl.deploy and checkComplete
                Ales Justin Master

                Regarding the single deployment process api.
                Is adding proper locks (similar to what we do in AbstractController) the way to go, or should just the relevant methods be re-written to support single deployment in correct way with regard to topLevelDeployments, allDeployments, ... collections in MainDeployerImpl?

                • 5. Re: MainDeployerImpl.deploy and checkComplete
                  Adrian Brock Master

                  Actually, thinking about it the signatures should really be:

                   void deploy(Deployment... deployment) throws DeploymentException
                   void undeploy(Deployment... deployment) throws DeploymentException
                  


                  So you can deploy multiple deployment together in a thread safe
                  and atomic way.

                  This will make the implementation a bit more complicated, buit it will be
                  more useful when you need to deploy mutiple deployments that are related.
                  This happens a lot in the JBossAS testsuite.
                  e.g.
                  getDeploySetup(MyTestCase.class, "queues-service.xml, test.ear");
                  


                  The ... means it will also work for a single deployment as well.

                  • 6. Re: MainDeployerImpl.deploy and checkComplete
                    Adrian Brock Master

                     

                    "alesj" wrote:
                    Regarding the single deployment process api.
                    Is adding proper locks (similar to what we do in AbstractController) the way to go, or should just the relevant methods be re-written to support single deployment in correct way with regard to topLevelDeployments, allDeployments, ... collections in MainDeployerImpl?


                    The existing process() should be re-written to prebuild the list of deployments
                    then invoke a new process(Deployment... deployments).