Uploaded image for project: 'Slider'
  1. Slider
  2. SLIDER-773

Add co-processor support for app packages

    Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: Slider 0.60
    • Fix Version/s: Slider 0.80
    • Component/s: app-package, client
    • Labels:
      None
    • Sprint:
      Slider April #1

      Description

      It is typical for applications to allow plugins/co-processors that are essentially a set of additional jar files in the classpath and optionally a set of config files or config changes.

      Current, slider app packages can handle additional config changes/entries very well. Additional configs files can be added as well but it is not easy if the config files include parameters that need to be resolved by the agent. This requires app package changes. Dropping additional jar files into the class path is not easy and requires app package changes.

      It is not efficient to modify the app package to support such plugins. App packaging and create command should be modified such that the user can dynamically specify additional jars, config files, configs etc.

      Specific scenarios are modifying HBase to add support for Phoenix or Ranger.

      1. 773-suggest.txt
        2 kB
        Ted Yu
      2. coprocessor.patch
        74 kB
        thomas liu
      3. coprocessor.patch
        84 kB
        thomas liu
      4. coprocessor-after.patch
        17 kB
        thomas liu
      5. coprocessor-apri-4th.patch
        70 kB
        thomas liu
      6. Co-processorSupport.pdf
        75 kB
        Sumit Mohanty
      7. SLIDER-773_review_comments_part2.patch
        3 kB
        Gour Saha
      8. SLIDER-773_review_comments.patch
        60 kB
        Gour Saha

        Issue Links

          Activity

          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit a008aee20340b3395202f126b39254540917551c in incubator-slider's branch refs/heads/develop from Gour Saha
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=a008aee ]

          SLIDER-773 Add co-processor support for app packages - fix fun tests (Thomas Liu via gourksaha)

          Show
          jira-bot ASF subversion and git services added a comment - Commit a008aee20340b3395202f126b39254540917551c in incubator-slider's branch refs/heads/develop from Gour Saha [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=a008aee ] SLIDER-773 Add co-processor support for app packages - fix fun tests (Thomas Liu via gourksaha)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1df52b8050450c6d21527e575fe4f5d62f4c7ad3 in incubator-slider's branch refs/heads/develop from Gour Saha
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=1df52b8 ]

          SLIDER-773 Add co-processor support for app packages - additional fix for windows (Thomas Liu via gourksaha)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1df52b8050450c6d21527e575fe4f5d62f4c7ad3 in incubator-slider's branch refs/heads/develop from Gour Saha [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=1df52b8 ] SLIDER-773 Add co-processor support for app packages - additional fix for windows (Thomas Liu via gourksaha)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e18c585eb032da47b04fb6473466cb2f0d6cfdae in incubator-slider's branch refs/heads/develop from Gour Saha
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=e18c585 ]

          SLIDER-773 Add co-processor support for app packages - fix windows tests (Thomas Liu via gourksaha)

          Show
          jira-bot ASF subversion and git services added a comment - Commit e18c585eb032da47b04fb6473466cb2f0d6cfdae in incubator-slider's branch refs/heads/develop from Gour Saha [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=e18c585 ] SLIDER-773 Add co-processor support for app packages - fix windows tests (Thomas Liu via gourksaha)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 31bc38ebbe044d8c87cc0b9903b679725d43fa14 in incubator-slider's branch refs/heads/releases/slider-0.80.0-incubating from Gour Saha
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=31bc38e ]

          SLIDER-773 Add co-processor support for app packages - fix fun tests (Thomas Liu via gourksaha)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 31bc38ebbe044d8c87cc0b9903b679725d43fa14 in incubator-slider's branch refs/heads/releases/slider-0.80.0-incubating from Gour Saha [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=31bc38e ] SLIDER-773 Add co-processor support for app packages - fix fun tests (Thomas Liu via gourksaha)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 64d3a2c9656fef34b450b3f024eab45ea075fb80 in incubator-slider's branch refs/heads/develop from Gour Saha
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=64d3a2c ]

          SLIDER-773 Add co-processor support for app packages - fix fun tests (Thomas Liu via gourksaha)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 64d3a2c9656fef34b450b3f024eab45ea075fb80 in incubator-slider's branch refs/heads/develop from Gour Saha [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=64d3a2c ] SLIDER-773 Add co-processor support for app packages - fix fun tests (Thomas Liu via gourksaha)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit b97a861135eaa8577ec27a4201e0c8598c0ff9bd in incubator-slider's branch refs/heads/develop from Gour Saha
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=b97a861 ]

          SLIDER-773 Add co-processor support for app packages - unit and fun tests (Thomas Liu via gourksaha)

          Show
          jira-bot ASF subversion and git services added a comment - Commit b97a861135eaa8577ec27a4201e0c8598c0ff9bd in incubator-slider's branch refs/heads/develop from Gour Saha [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=b97a861 ] SLIDER-773 Add co-processor support for app packages - unit and fun tests (Thomas Liu via gourksaha)
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Can we do something with no JARs? Just some python scripts? the source distribution is required to have no binaries at all..with .py scripts we can just have the build create & publish a new coprocessor-test JAR

          Show
          stevel@apache.org Steve Loughran added a comment - Can we do something with no JARs? Just some python scripts? the source distribution is required to have no binaries at all..with .py scripts we can just have the build create & publish a new coprocessor-test JAR
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Two of the slider-core tests are failing on my machine

          Tests in error: 
            TestAddonPackage.testNoSuchPackageFile:106->SliderTestUtils.launch:953 » BadCommandArguments
          
          Remover received java.lang.InterruptedException: sleep interrupted
          2015-04-24 19:28:04,455 [Thread-1] INFO  resourcemanager.ResourceManager (ResourceManager.java:transitionToStandby(1068)) - Transitioned to standby state
          Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 6.601 sec <<< FAILURE! - in org.apache.slider.providers.agent.TestAddonPackage
          testNoSuchPackageFile(org.apache.slider.providers.agent.TestAddonPackage)  Time elapsed: 5.308 sec  <<< ERROR!
          org.apache.slider.core.exceptions.BadCommandArgumentsException: Unable to access supplied pkg file at /Users/stevel/Projects/Hortonworks/Projects/slider/slider-core/src/test/app_packages/test_addon_pkg/master-app-pkg.zip
          	at org.apache.slider.client.SliderClient.actionPackageInstall(SliderClient.java:1430)
          	at org.apache.slider.client.SliderClient.actionPackage(SliderClient.java:1292)
          	at org.apache.slider.client.SliderClient.exec(SliderClient.java:482)
          	at org.apache.slider.client.SliderClient.runService(SliderClient.java:388)
          	at org.apache.slider.core.main.ServiceLauncher.launchService(ServiceLauncher.java:188)
          	at org.apache.slider.test.SliderTestUtils.launch(SliderTestUtils.groovy:953)
          	at org.apache.slider.test.SliderTestUtils$launch.call(Unknown Source)
          	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:45)
          	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:108)
          	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:136)
          	at org.apache.slider.providers.agent.TestAddonPackage.testNoSuchPackageFile(TestAddonPackage.groovy:106)
          
          
          
          
          Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 7.82 sec <<< FAILURE! - in org.apache.slider.providers.agent.TestAgentEcho
          testEchoOperationForAddonPackages(org.apache.slider.providers.agent.TestAgentEcho)  Time elapsed: 6.52 sec  <<< ERROR!
          org.apache.slider.core.exceptions.BadConfigException: Error retrieving metainfo data at /Users/yliu/.slider/package/FAKE/master-app-pkg.zip: java.io.FileNotFoundException: File /Users/yliu/.slider/package/FAKE/master-app-pkg.zip does not exist
          	at org.apache.slider.providers.agent.AgentClientProvider.getApplicationTags(AgentClientProvider.java:289)
          	at org.apache.slider.client.SliderClient.launchApplication(SliderClient.java:1993)
          	at org.apache.slider.client.SliderClient.startCluster(SliderClient.java:1864)
          	at org.apache.slider.client.SliderClient.actionCreate(SliderClient.java:716)
          	at org.apache.slider.client.SliderClient.exec(SliderClient.java:427)
          	at org.apache.slider.client.SliderClient.runService(SliderClient.java:388)
          	at org.apache.slider.core.main.ServiceLauncher.launchService(ServiceLauncher.java:188)
          	at org.apache.slider.test.SliderTestUtils.execSliderCommand(SliderTestUtils.groovy:939)
          	at org.apache.slider.test.SliderTestUtils.launchClientAgainstRM(SliderTestUtils.groovy:990)
          	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
          	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
          	at java.lang.reflect.Method.invoke(Method.java:606)
          	at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:90)
          	at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:324)
          	at groovy.lang.MetaClassImpl.invokeStaticMethod(MetaClassImpl.java:1462)
          	at org.codehaus.groovy.runtime.callsite.StaticMetaClassSite.callStatic(StaticMetaClassSite.java:62)
          	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallStatic(CallSiteArray.java:53)
          	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:189)
          	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:217)
          	at org.apache.slider.test.YarnMiniClusterTestBase.launchClientNoExitCodeCheck(YarnMiniClusterTestBase.groovy:378)
          	at org.apache.slider.test.YarnMiniClusterTestBase$launchClientNoExitCodeCheck$3.callCurrent(Unknown Source)
          	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:49)
          	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:149)
          	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:169)
          	at org.apache.slider.test.YarnMiniClusterTestBase.launchClientAgainstMiniMR(YarnMiniClusterTestBase.groovy:359)
          	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
          	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
          	at java.lang.reflect.Method.invoke(Method.java:606)
          	at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite$PogoCachedMethodSiteNoUnwrapNoCoerce.invoke(PogoMetaMethodSite.java:207)
          	at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite.callCurrent(PogoMetaMethodSite.java:56)
          	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:49)
          	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:149)
          	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:169)
          	at org.apache.slider.test.YarnMiniClusterTestBase.createOrBuildCluster(YarnMiniClusterTestBase.groovy:536)
          	at org.apache.slider.providers.agent.AgentTestBase.buildAgentCluster(AgentTestBase.groovy:151)
          	at org.apache.slider.providers.agent.TestAgentEcho.testEchoOperationForAddonPackages(TestAgentEcho.groovy:165)
          
          
          Results :
          
          Tests in error: 
            TestAgentEcho.testEchoOperationForAddonPackages:165->AgentTestBase.buildAgentCluster:151->YarnMiniClusterTestBase.createOrBuildCluster:536->YarnMiniClusterTestBase.launchClientAgainstMiniMR:359->YarnMiniClusterTestBase.launchClientNoExitCodeCheck:378->SliderTestUtils.launchClientAgainstRM:990->SliderTestUtils.execSliderCommand:939 » BadConfig
          
          Show
          stevel@apache.org Steve Loughran added a comment - Two of the slider-core tests are failing on my machine Tests in error: TestAddonPackage.testNoSuchPackageFile:106->SliderTestUtils.launch:953 » BadCommandArguments Remover received java.lang.InterruptedException: sleep interrupted 2015-04-24 19:28:04,455 [ Thread -1] INFO resourcemanager.ResourceManager (ResourceManager.java:transitionToStandby(1068)) - Transitioned to standby state Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 6.601 sec <<< FAILURE! - in org.apache.slider.providers.agent.TestAddonPackage testNoSuchPackageFile(org.apache.slider.providers.agent.TestAddonPackage) Time elapsed: 5.308 sec <<< ERROR! org.apache.slider.core.exceptions.BadCommandArgumentsException: Unable to access supplied pkg file at /Users/stevel/Projects/Hortonworks/Projects/slider/slider-core/src/test/app_packages/test_addon_pkg/master-app-pkg.zip at org.apache.slider.client.SliderClient.actionPackageInstall(SliderClient.java:1430) at org.apache.slider.client.SliderClient.actionPackage(SliderClient.java:1292) at org.apache.slider.client.SliderClient.exec(SliderClient.java:482) at org.apache.slider.client.SliderClient.runService(SliderClient.java:388) at org.apache.slider.core.main.ServiceLauncher.launchService(ServiceLauncher.java:188) at org.apache.slider.test.SliderTestUtils.launch(SliderTestUtils.groovy:953) at org.apache.slider.test.SliderTestUtils$launch.call(Unknown Source) at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:45) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:108) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:136) at org.apache.slider.providers.agent.TestAddonPackage.testNoSuchPackageFile(TestAddonPackage.groovy:106) Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 7.82 sec <<< FAILURE! - in org.apache.slider.providers.agent.TestAgentEcho testEchoOperationForAddonPackages(org.apache.slider.providers.agent.TestAgentEcho) Time elapsed: 6.52 sec <<< ERROR! org.apache.slider.core.exceptions.BadConfigException: Error retrieving metainfo data at /Users/yliu/.slider/ package /FAKE/master-app-pkg.zip: java.io.FileNotFoundException: File /Users/yliu/.slider/ package /FAKE/master-app-pkg.zip does not exist at org.apache.slider.providers.agent.AgentClientProvider.getApplicationTags(AgentClientProvider.java:289) at org.apache.slider.client.SliderClient.launchApplication(SliderClient.java:1993) at org.apache.slider.client.SliderClient.startCluster(SliderClient.java:1864) at org.apache.slider.client.SliderClient.actionCreate(SliderClient.java:716) at org.apache.slider.client.SliderClient.exec(SliderClient.java:427) at org.apache.slider.client.SliderClient.runService(SliderClient.java:388) at org.apache.slider.core.main.ServiceLauncher.launchService(ServiceLauncher.java:188) at org.apache.slider.test.SliderTestUtils.execSliderCommand(SliderTestUtils.groovy:939) at org.apache.slider.test.SliderTestUtils.launchClientAgainstRM(SliderTestUtils.groovy:990) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:606) at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:90) at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:324) at groovy.lang.MetaClassImpl.invokeStaticMethod(MetaClassImpl.java:1462) at org.codehaus.groovy.runtime.callsite.StaticMetaClassSite.callStatic(StaticMetaClassSite.java:62) at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallStatic(CallSiteArray.java:53) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:189) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:217) at org.apache.slider.test.YarnMiniClusterTestBase.launchClientNoExitCodeCheck(YarnMiniClusterTestBase.groovy:378) at org.apache.slider.test.YarnMiniClusterTestBase$launchClientNoExitCodeCheck$3.callCurrent(Unknown Source) at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:49) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:149) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:169) at org.apache.slider.test.YarnMiniClusterTestBase.launchClientAgainstMiniMR(YarnMiniClusterTestBase.groovy:359) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:606) at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite$PogoCachedMethodSiteNoUnwrapNoCoerce.invoke(PogoMetaMethodSite.java:207) at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite.callCurrent(PogoMetaMethodSite.java:56) at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:49) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:149) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:169) at org.apache.slider.test.YarnMiniClusterTestBase.createOrBuildCluster(YarnMiniClusterTestBase.groovy:536) at org.apache.slider.providers.agent.AgentTestBase.buildAgentCluster(AgentTestBase.groovy:151) at org.apache.slider.providers.agent.TestAgentEcho.testEchoOperationForAddonPackages(TestAgentEcho.groovy:165) Results : Tests in error: TestAgentEcho.testEchoOperationForAddonPackages:165->AgentTestBase.buildAgentCluster:151->YarnMiniClusterTestBase.createOrBuildCluster:536->YarnMiniClusterTestBase.launchClientAgainstMiniMR:359->YarnMiniClusterTestBase.launchClientNoExitCodeCheck:378->SliderTestUtils.launchClientAgainstRM:990->SliderTestUtils.execSliderCommand:939 » BadConfig
          Hide
          thomas_liu thomas liu added a comment -

          Thank you for your comment!

          The hbase tar there was originally used to test Slider, with add on package's python script, can copy the file over to the right directory. I should replace it with some small jars

          Building the zip files online is definitely a good idea. I will modify my current test cases to do that.

          Thanks again!

          Show
          thomas_liu thomas liu added a comment - Thank you for your comment! The hbase tar there was originally used to test Slider, with add on package's python script, can copy the file over to the right directory. I should replace it with some small jars Building the zip files online is definitely a good idea. I will modify my current test cases to do that. Thanks again!
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I'm getting the tests running.
          the add-on test .zip files in slider-core/src/test/app_packages/test_addon_pkg are trouble as they stand.

          • multiple 79MB files is making for a big git repo
          • we can't do zip files in our source distribution...that caused lots of fun last time and we promised never to to do it again.

          We don't need the hbase tar in these, do we? So we could just have a minimal set of zips with nothing but metadata and .py files in? In which case we can actually get the build process to build these as part of the test run (we can use the <ant> plugin to build whatever zip files we want in the package phase).

          Show
          stevel@apache.org Steve Loughran added a comment - I'm getting the tests running. the add-on test .zip files in slider-core/src/test/app_packages/test_addon_pkg are trouble as they stand. multiple 79MB files is making for a big git repo we can't do zip files in our source distribution...that caused lots of fun last time and we promised never to to do it again. We don't need the hbase tar in these, do we? So we could just have a minimal set of zips with nothing but metadata and .py files in? In which case we can actually get the build process to build these as part of the test run (we can use the <ant> plugin to build whatever zip files we want in the package phase).
          Hide
          gsaha Gour Saha added a comment -

          Review patches have been checked in.

          Show
          gsaha Gour Saha added a comment - Review patches have been checked in.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 211cb28c5ee231bb9899db9d0c85da60aab88a21 in incubator-slider's branch refs/heads/develop from Gour Saha
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=211cb28 ]

          SLIDER-773. Add co-processor support for app packages (code review changes - Thomas Liu via gourksaha)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 211cb28c5ee231bb9899db9d0c85da60aab88a21 in incubator-slider's branch refs/heads/develop from Gour Saha [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=211cb28 ] SLIDER-773 . Add co-processor support for app packages (code review changes - Thomas Liu via gourksaha)
          Hide
          gsaha Gour Saha added a comment -

          Don't worry it is taken care of in part 2 of the review patch. The null argument will be there.

          Show
          gsaha Gour Saha added a comment - Don't worry it is taken care of in part 2 of the review patch. The null argument will be there.
          Hide
          thomas_liu thomas liu added a comment - - edited

          Thank you for your patch! It looks good to me.
          Only one thing that I don't know why happen:

          @@ -999,7 +999,7 @@ public class AgentProviderService extends AbstractProviderService implements

          } catch (SliderException e)

          Unknown macro: { log.warn("Component instance failed operation.", e);- componentStatus.applyCommandResult(CommandResult.FAILED, command);+ componentStatus.applyCommandResult(CommandResult.FAILED, command, null); }

          log.debug("Heartbeat response: " + response);

          On my side, it is already

          componentStatus.applyCommandResult(CommandResult.FAILED, command, null);

          and it is not what I modified recently...

          Show
          thomas_liu thomas liu added a comment - - edited Thank you for your patch! It looks good to me. Only one thing that I don't know why happen: @@ -999,7 +999,7 @@ public class AgentProviderService extends AbstractProviderService implements } catch (SliderException e) Unknown macro: { log.warn("Component instance failed operation.", e);- componentStatus.applyCommandResult(CommandResult.FAILED, command);+ componentStatus.applyCommandResult(CommandResult.FAILED, command, null); } log.debug("Heartbeat response: " + response); On my side, it is already componentStatus.applyCommandResult(CommandResult.FAILED, command, null); and it is not what I modified recently...
          Hide
          gsaha Gour Saha added a comment -

          Uploading another patch with additional changes based on your comments and mine. Please review this small part2 patch, and I will check in both part 1 and part 2.

          Show
          gsaha Gour Saha added a comment - Uploading another patch with additional changes based on your comments and mine. Please review this small part2 patch, and I will check in both part 1 and part 2.
          Hide
          gsaha Gour Saha added a comment -

          Right now, it IS calling the non-pkg version (by passing in null as the third param)

          You are right. I put the null back.

          This is an interesting case. This method was originally written by Sumit to define the add on package file name on HDFS. However, line 82"String targetName = appDefinition.pkgName;" was originally assigned some other values which caused appConfig has a different add on package file name than the actual one on HDFS. That's why I modified 82 to be assigned appDefinition.pkgName.
          I also noticed the if block assign this name again, but it contains more logic in case the user provided a folder, vs a file as the add on package file. so we should leave the if block there

          Actually I was not referring to removing the if block. I was saying that if the logic is correct as it exists today, then we don't need the line targetName = appDefinition.pkgName; inside the if block. Everything else stays as is. Also, the log statements outside the if block should be modified accordingly to avoid confusion to a developer that appDefinition.pkgName should have changed inside.

          I believe what you mentioned is a general principal for declaring variables.
          However, in our case, the declaration must be of type TreeMap instead of a normal Map, because we need an ordered map so that we can traverse it in order

          I understand your point, but you need it only when you call methods which are exposed by the implementation class only, say lowerEntry or lowerKey of TreeMap. Since you are not making any such calls, you don't need the declaration variable to be of type TreeMap.

          Show
          gsaha Gour Saha added a comment - Right now, it IS calling the non-pkg version (by passing in null as the third param) You are right. I put the null back. This is an interesting case. This method was originally written by Sumit to define the add on package file name on HDFS. However, line 82"String targetName = appDefinition.pkgName;" was originally assigned some other values which caused appConfig has a different add on package file name than the actual one on HDFS. That's why I modified 82 to be assigned appDefinition.pkgName. I also noticed the if block assign this name again, but it contains more logic in case the user provided a folder, vs a file as the add on package file. so we should leave the if block there Actually I was not referring to removing the if block. I was saying that if the logic is correct as it exists today, then we don't need the line targetName = appDefinition.pkgName; inside the if block. Everything else stays as is. Also, the log statements outside the if block should be modified accordingly to avoid confusion to a developer that appDefinition.pkgName should have changed inside. I believe what you mentioned is a general principal for declaring variables. However, in our case, the declaration must be of type TreeMap instead of a normal Map, because we need an ordered map so that we can traverse it in order I understand your point, but you need it only when you call methods which are exposed by the implementation class only, say lowerEntry or lowerKey of TreeMap. Since you are not making any such calls, you don't need the declaration variable to be of type TreeMap.
          Hide
          gsaha Gour Saha added a comment -

          You are right. If validation has been done before this code comes into play, then we do not need additional null checks.

          Show
          gsaha Gour Saha added a comment - You are right. If validation has been done before this code comes into play, then we do not need additional null checks.
          Hide
          thomas_liu thomas liu added a comment - - edited

          This is a good point. I was hesitating over this as well.
          Right now, if there are add on packages to process, once they pass initial validation, pkgStatus will not be empty and contain the key for sure. If there are no packages to process, we would not visit this map at all.
          But I do agree that we should probably have some null pointer checking here. When I look around AgentProviderService, it looks like for several places, we are not doing this checking but confidently use the variable got from the map. For example, in handleHeartBeat(), line 891, installCmd = compCmd. That's why I followed this pattern
          Please let me know what you think. If necessary, we can create a ticket to fix all those cases

          Show
          thomas_liu thomas liu added a comment - - edited This is a good point. I was hesitating over this as well. Right now, if there are add on packages to process, once they pass initial validation, pkgStatus will not be empty and contain the key for sure. If there are no packages to process, we would not visit this map at all. But I do agree that we should probably have some null pointer checking here. When I look around AgentProviderService, it looks like for several places, we are not doing this checking but confidently use the variable got from the map. For example, in handleHeartBeat(), line 891, installCmd = compCmd. That's why I followed this pattern Please let me know what you think. If necessary, we can create a ticket to fix all those cases
          Hide
          thomas_liu thomas liu added a comment - - edited

          Hi, Gour:

          Thank you very much for your comments!

          I tested it manually and it works for normal add on packages scenarios

          Below are my responses to the code:

          Potential bugs -
          In line 1002 of AgentProviderService.java was the pkg or non-pkg version of componentStatus.applyCommandResult supposed to be called? I think we should call the non-pkg version.
          } catch (SliderException e)

          Unknown macro: { log.warn("Component instance failed operation.", e); componentStatus.applyCommandResult(CommandResult.FAILED, command, null); }

          Right now, it IS calling the non-pkg version (by passing in null as the third param)

          Starting with line 82 of AppDefinitionPersister.java, appDefinition.pkgName is not changed in the following if block. Based on the logging, seems like it is supposed to change. Is it a potential bug?
          String targetName = appDefinition.pkgName;
          log.debug("Initial package name: " + targetName);
          if (appDefinition.appDefPkgOrFolder.isDirectory()) {
          log.info("Processing app package/folder {} for {}",
          appDefinition.appDefPkgOrFolder.getAbsolutePath(),
          appDefinition.pkgName);
          File tmpDir = Files.createTempDir();
          File zipFile = new File(tmpDir.getCanonicalPath(), File.separator + appDefinition.pkgName);
          SliderUtils.zipFolder(appDefinition.appDefPkgOrFolder, zipFile);

          src = zipFile;
          targetName = appDefinition.pkgName;
          }
          log.debug("Final package name: " + targetName);

          This is an interesting case. This method was originally written by Sumit to define the add on package file name on HDFS. However, line 82"String targetName = appDefinition.pkgName;" was originally assigned some other values which caused appConfig has a different add on package file name than the actual one on HDFS. That's why I modified 82 to be assigned appDefinition.pkgName.

          I also noticed the if block assign this name again, but it contains more logic in case the user provided a folder, vs a file as the add on package file. so we should leave the if block there

          Agree with all of the comments below, with exceptions in between:

          Themes of other issues -
          Avoid unnecessary new lines in both python and java files
          Avoid spaces in new lines in both python and java files
          Start log message texts with a capitalized word (for consistency in Slider code-base)
          As Ted Yu already mentioned, avoid long lines (wrap beyond 80 chars)
          As Steve Loughran pointed out, use formatter versions for logging instead of string concat. It is more efficient and avoids unnecessary creation of objects for debug level logs say, when logging is set at a higher level.
          Avoid logging at info level in heartbeat flows (in both agent provider and python side) for NOP/STATUS command scenarios. Use AgentToggleLogger.py on the python side to see how you can log once for STATUS commands and then switch to silent mode, until a non-STATUS command shows up again.
          Avoid repetition of code for overloaded methods or constructors. Typically most of the logic can be written in one method/constructor and call that method/constructor with appropriate placeholder values from the other overloaded methods/constructors.
          There should be a space between if and ( & between ) and } for if statements
          Remove unnecessary imports before creating a patch
          If a patch is a large one, typically avoid making changes to a file which has cosmetic changes only. The cosmetic change can always be made as a separate patch with a corresponding JIRA.
          Declaration of variables or method signatures should not use implementation classes like -
          TreeMap<String, State> pkgStatuses = new TreeMap<String, State>();
          instead use
          Map<String, State> pkgStatuses = new TreeMap<String, State>();

          I believe what you mentioned is a general principal for declaring variables.
          However, in our case, the declaration must be of type TreeMap instead of a normal Map, because we need an ordered map so that we can traverse it in order

          As Ted Yu mentioned Component.MASTER_PACKAGE_NAME.equals(pkg) covers the check for pkg.isEmpty()
          In ComponentInstanceState.java as Ted Yu mentioned, you should use pkgStatuses.entrySet() instead of pkgStatuses.keySet() as both key and value are accessed in the loop.
          In ComponentInstanceState.java is the following line required just before the return statement (around line 234)?
          nextPkgToInstall = null;
          Try to define all accessor/setter methods before methods like toString, hashCode and equals
          Do not repeat getters/setters in concrete class like Component.java which are already defined in abstract super classes
          Do not checkin code with default stub codes like // TODO Auto-generated method stub

          Show
          thomas_liu thomas liu added a comment - - edited Hi, Gour: Thank you very much for your comments! I tested it manually and it works for normal add on packages scenarios Below are my responses to the code: Potential bugs - In line 1002 of AgentProviderService.java was the pkg or non-pkg version of componentStatus.applyCommandResult supposed to be called? I think we should call the non-pkg version. } catch (SliderException e) Unknown macro: { log.warn("Component instance failed operation.", e); componentStatus.applyCommandResult(CommandResult.FAILED, command, null); } Right now, it IS calling the non-pkg version (by passing in null as the third param) Starting with line 82 of AppDefinitionPersister.java, appDefinition.pkgName is not changed in the following if block. Based on the logging, seems like it is supposed to change. Is it a potential bug? String targetName = appDefinition.pkgName; log.debug("Initial package name: " + targetName); if (appDefinition.appDefPkgOrFolder.isDirectory()) { log.info("Processing app package/folder {} for {}", appDefinition.appDefPkgOrFolder.getAbsolutePath(), appDefinition.pkgName); File tmpDir = Files.createTempDir(); File zipFile = new File(tmpDir.getCanonicalPath(), File.separator + appDefinition.pkgName); SliderUtils.zipFolder(appDefinition.appDefPkgOrFolder, zipFile); src = zipFile; targetName = appDefinition.pkgName; } log.debug("Final package name: " + targetName); This is an interesting case. This method was originally written by Sumit to define the add on package file name on HDFS. However, line 82"String targetName = appDefinition.pkgName;" was originally assigned some other values which caused appConfig has a different add on package file name than the actual one on HDFS. That's why I modified 82 to be assigned appDefinition.pkgName. I also noticed the if block assign this name again, but it contains more logic in case the user provided a folder, vs a file as the add on package file. so we should leave the if block there Agree with all of the comments below, with exceptions in between: Themes of other issues - Avoid unnecessary new lines in both python and java files Avoid spaces in new lines in both python and java files Start log message texts with a capitalized word (for consistency in Slider code-base) As Ted Yu already mentioned, avoid long lines (wrap beyond 80 chars) As Steve Loughran pointed out, use formatter versions for logging instead of string concat. It is more efficient and avoids unnecessary creation of objects for debug level logs say, when logging is set at a higher level. Avoid logging at info level in heartbeat flows (in both agent provider and python side) for NOP/STATUS command scenarios. Use AgentToggleLogger.py on the python side to see how you can log once for STATUS commands and then switch to silent mode, until a non-STATUS command shows up again. Avoid repetition of code for overloaded methods or constructors. Typically most of the logic can be written in one method/constructor and call that method/constructor with appropriate placeholder values from the other overloaded methods/constructors. There should be a space between if and ( & between ) and } for if statements Remove unnecessary imports before creating a patch If a patch is a large one, typically avoid making changes to a file which has cosmetic changes only. The cosmetic change can always be made as a separate patch with a corresponding JIRA. Declaration of variables or method signatures should not use implementation classes like - TreeMap<String, State> pkgStatuses = new TreeMap<String, State>(); instead use Map<String, State> pkgStatuses = new TreeMap<String, State>(); I believe what you mentioned is a general principal for declaring variables. However, in our case, the declaration must be of type TreeMap instead of a normal Map, because we need an ordered map so that we can traverse it in order As Ted Yu mentioned Component.MASTER_PACKAGE_NAME.equals(pkg) covers the check for pkg.isEmpty() In ComponentInstanceState.java as Ted Yu mentioned, you should use pkgStatuses.entrySet() instead of pkgStatuses.keySet() as both key and value are accessed in the loop. In ComponentInstanceState.java is the following line required just before the return statement (around line 234)? nextPkgToInstall = null; Try to define all accessor/setter methods before methods like toString, hashCode and equals Do not repeat getters/setters in concrete class like Component.java which are already defined in abstract super classes Do not checkin code with default stub codes like // TODO Auto-generated method stub
          Hide
          gsaha Gour Saha added a comment -

          Please use the latest review patch as of Apr 15, 8:30am PST.

          Show
          gsaha Gour Saha added a comment - Please use the latest review patch as of Apr 15, 8:30am PST.
          Hide
          gsaha Gour Saha added a comment -

          My bad on point 12 above. Seems like I missed the point that the if block should not be executed if pkg is empty string. I changed the code slightly to use StringUtils.isNotEmpty(pkg). thomas liu is pkg validated such that pkgStatuses.get(pkg) will never return a null? I mean, if pkg is not empty, will it be always true that pkgStatuses will contain an entry for pkg? If not, then we will run into NPE.

          Show
          gsaha Gour Saha added a comment - My bad on point 12 above. Seems like I missed the point that the if block should not be executed if pkg is empty string. I changed the code slightly to use StringUtils.isNotEmpty(pkg) . thomas liu is pkg validated such that pkgStatuses.get(pkg) will never return a null? I mean, if pkg is not empty, will it be always true that pkgStatuses will contain an entry for pkg? If not, then we will run into NPE.
          Hide
          gsaha Gour Saha added a comment -

          Uploading a patch SLIDER-773_review_comments.patch which covers my review feedback. Overall the patch looks good. Just couple of potential bug which needs to be reviewed by thomas liu. Others are mostly formatting and cosmetic related, but important, to avoid creating inconsistencies in the code base.

          The overall theme of the cosmetic changes are provided below. Feel free to take this patch and make necessary changes and re-upload. I can check in the patch for you. I did not get a chance to run tests on top of my review patch, so please do so.

          Potential bugs -

          1. In line 1002 of AgentProviderService.java was the pkg or non-pkg version of componentStatus.applyCommandResult supposed to be called? I think we should call the non-pkg version.
                } catch (SliderException e) {
                  log.warn("Component instance failed operation.", e);
                  componentStatus.applyCommandResult(CommandResult.FAILED, command, null);
                }
            
          2. Starting with line 82 of AppDefinitionPersister.java, appDefinition.pkgName is not changed in the following if block. Based on the logging, seems like it is supposed to change. Is it a potential bug?
                String targetName = appDefinition.pkgName;
                log.debug("Initial package name: " + targetName);
                if (appDefinition.appDefPkgOrFolder.isDirectory()) {
                  log.info("Processing app package/folder {} for {}",
                           appDefinition.appDefPkgOrFolder.getAbsolutePath(),
                           appDefinition.pkgName);
                  File tmpDir = Files.createTempDir();
                  File zipFile = new File(tmpDir.getCanonicalPath(), File.separator + appDefinition.pkgName);
                  SliderUtils.zipFolder(appDefinition.appDefPkgOrFolder, zipFile);
            
                  src = zipFile;
                  targetName = appDefinition.pkgName;
                }
                log.debug("Final package name: " + targetName);
            

          Themes of other issues -

          1. Avoid unnecessary new lines in both python and java files
          2. Avoid spaces in new lines in both python and java files
          3. Start log message texts with a capitalized word (for consistency in Slider code-base)
          4. As Ted Yu already mentioned, avoid long lines (wrap beyond 80 chars)
          5. As Steve Loughran pointed out, use formatter versions for logging instead of string concat. It is more efficient and avoids unnecessary creation of objects for debug level logs say, when logging is set at a higher level.
          6. Avoid logging at info level in heartbeat flows (in both agent provider and python side) for NOP/STATUS command scenarios. Use AgentToggleLogger.py on the python side to see how you can log once for STATUS commands and then switch to silent mode, until a non-STATUS command shows up again.
          7. Avoid repetition of code for overloaded methods or constructors. Typically most of the logic can be written in one method/constructor and call that method/constructor with appropriate placeholder values from the other overloaded methods/constructors.
          8. There should be a space between if and ( & between ) and } for if statements
          9. Remove unnecessary imports before creating a patch
          10. If a patch is a large one, typically avoid making changes to a file which has cosmetic changes only. The cosmetic change can always be made as a separate patch with a corresponding JIRA.
          11. Declaration of variables or method signatures should not use implementation classes like -
            TreeMap<String, State> pkgStatuses = new TreeMap<String, State>();
            

            instead use

            Map<String, State> pkgStatuses = new TreeMap<String, State>();
            
          12. As Ted Yu mentioned Component.MASTER_PACKAGE_NAME.equals(pkg) covers the check for pkg.isEmpty()
          13. In ComponentInstanceState.java as Ted Yu mentioned, you should use pkgStatuses.entrySet() instead of pkgStatuses.keySet() as both key and value are accessed in the loop.
          14. In ComponentInstanceState.java is the following line required just before the return statement (around line 234)?
                nextPkgToInstall = null;
            
          15. Try to define all accessor/setter methods before methods like toString, hashCode and equals
          16. Do not repeat getters/setters in concrete class like Component.java which are already defined in abstract super classes
          17. Do not checkin code with default stub codes like // TODO Auto-generated method stub
          Show
          gsaha Gour Saha added a comment - Uploading a patch SLIDER-773 _review_comments.patch which covers my review feedback. Overall the patch looks good. Just couple of potential bug which needs to be reviewed by thomas liu . Others are mostly formatting and cosmetic related, but important, to avoid creating inconsistencies in the code base. The overall theme of the cosmetic changes are provided below. Feel free to take this patch and make necessary changes and re-upload. I can check in the patch for you. I did not get a chance to run tests on top of my review patch, so please do so. Potential bugs - In line 1002 of AgentProviderService.java was the pkg or non-pkg version of componentStatus.applyCommandResult supposed to be called? I think we should call the non-pkg version. } catch (SliderException e) { log.warn( "Component instance failed operation." , e); componentStatus.applyCommandResult(CommandResult.FAILED, command, null ); } Starting with line 82 of AppDefinitionPersister.java , appDefinition.pkgName is not changed in the following if block. Based on the logging, seems like it is supposed to change. Is it a potential bug? String targetName = appDefinition.pkgName; log.debug( "Initial package name: " + targetName); if (appDefinition.appDefPkgOrFolder.isDirectory()) { log.info( "Processing app package /folder {} for {}" , appDefinition.appDefPkgOrFolder.getAbsolutePath(), appDefinition.pkgName); File tmpDir = Files.createTempDir(); File zipFile = new File(tmpDir.getCanonicalPath(), File.separator + appDefinition.pkgName); SliderUtils.zipFolder(appDefinition.appDefPkgOrFolder, zipFile); src = zipFile; targetName = appDefinition.pkgName; } log.debug( "Final package name: " + targetName); Themes of other issues - Avoid unnecessary new lines in both python and java files Avoid spaces in new lines in both python and java files Start log message texts with a capitalized word (for consistency in Slider code-base) As Ted Yu already mentioned, avoid long lines (wrap beyond 80 chars) As Steve Loughran pointed out, use formatter versions for logging instead of string concat. It is more efficient and avoids unnecessary creation of objects for debug level logs say, when logging is set at a higher level. Avoid logging at info level in heartbeat flows (in both agent provider and python side) for NOP/STATUS command scenarios. Use AgentToggleLogger.py on the python side to see how you can log once for STATUS commands and then switch to silent mode, until a non-STATUS command shows up again. Avoid repetition of code for overloaded methods or constructors. Typically most of the logic can be written in one method/constructor and call that method/constructor with appropriate placeholder values from the other overloaded methods/constructors. There should be a space between if and ( & between ) and } for if statements Remove unnecessary imports before creating a patch If a patch is a large one, typically avoid making changes to a file which has cosmetic changes only. The cosmetic change can always be made as a separate patch with a corresponding JIRA. Declaration of variables or method signatures should not use implementation classes like - TreeMap< String , State> pkgStatuses = new TreeMap< String , State>(); instead use Map< String , State> pkgStatuses = new TreeMap< String , State>(); As Ted Yu mentioned Component.MASTER_PACKAGE_NAME.equals(pkg) covers the check for pkg.isEmpty() In ComponentInstanceState.java as Ted Yu mentioned, you should use pkgStatuses.entrySet() instead of pkgStatuses.keySet() as both key and value are accessed in the loop. In ComponentInstanceState.java is the following line required just before the return statement (around line 234)? nextPkgToInstall = null ; Try to define all accessor/setter methods before methods like toString, hashCode and equals Do not repeat getters/setters in concrete class like Component.java which are already defined in abstract super classes Do not checkin code with default stub codes like // TODO Auto-generated method stub
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit fa73aee3a38ac9cf803bbf168f45723698ef0789 in incubator-slider's branch refs/heads/develop from Ted Yu
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=fa73aee ]

          SLIDER-773 Addendum which renames AddoPackageMetainfoParser (Thomas Liu)

          Show
          jira-bot ASF subversion and git services added a comment - Commit fa73aee3a38ac9cf803bbf168f45723698ef0789 in incubator-slider's branch refs/heads/develop from Ted Yu [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=fa73aee ] SLIDER-773 Addendum which renames AddoPackageMetainfoParser (Thomas Liu)
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Went through the above attachment - looks fine to me.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Went through the above attachment - looks fine to me.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Sumit Mohanty, Gour Saha - can you take a look at the python/agent side of this?

          Show
          stevel@apache.org Steve Loughran added a comment - Sumit Mohanty , Gour Saha - can you take a look at the python/agent side of this?
          Hide
          thomas_liu thomas liu added a comment -

          Sounds good. I have incorporated all your suggestion into the latest patch 'coprocessor-after.patch'

          Thank you!

          Show
          thomas_liu thomas liu added a comment - Sounds good. I have incorporated all your suggestion into the latest patch 'coprocessor-after.patch' Thank you!
          Hide
          thomas_liu thomas liu added a comment -

          Adopting Ted's comments

          Show
          thomas_liu thomas liu added a comment - Adopting Ted's comments
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Fine with most of the responses above.

          shall we use two loops to explicitly show a higher rank on checking state in INSTALLING?

          To me, using one loop still conveys the above notion - as shown in my attachment this morning. We can add comment before the first return:

                     nextPkgToInstall = pkg;
                     return Command.NOP;
          

          Saying that state of INSTALLING takes precedence over INIT state.
          What do you think ?

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Fine with most of the responses above. shall we use two loops to explicitly show a higher rank on checking state in INSTALLING? To me, using one loop still conveys the above notion - as shown in my attachment this morning. We can add comment before the first return: nextPkgToInstall = pkg; return Command.NOP; Saying that state of INSTALLING takes precedence over INIT state. What do you think ?
          Hide
          thomas_liu thomas liu added a comment - - edited

          Thank you for your comments.
          Here are my response to them:

          Some minor comments:
          + log.debug("metainfo map for master and addon is: {}",
          + packageMetainfo.toString());
          The contents of packageMetainfo have been logged in the prior while loop. Is the above needed ?

          This one is used to make sure all packages' metainfo is read, which is not helped by the one in the loop.
          The one in the loop can be ignored. I'm removing it from the loop

          + log.debug("for comp " + roleName + " pkg " + componentStatus.getNextPkgToInstall() + " issuing " + command.toString());
          Wrap long line. In HBase, line length is limited to 100.

          Sounds good. Wrapped.

          + ComponentCommand installCmd = null;
          + for (ComponentCommand compCmd : comp.getCommands()) {
          + if (compCmd.getName().equals("INSTALL"))

          { + installCmd = compCmd; + }

          + }
          + addInstallCommand(roleName, containerId, response, null,
          + installCmd, timeout, nextPkgToInstall);
          What if 'INSTALL' command isn't found in the for loop above ?

          I'm following the same pattern for master package. Please refer to "if (command == Command.INSTALL) "block above. Validation on the metainfo guarantees that there must be python script or INSTALL command

          + String appDef, boolean addonPackage) throws IOException, BadConfigException {
          Long line.

          Wrapped

          + * @param pkg TODO
          Please finish javadoc

          Done

          + metainfoParser = new AddoPackageMetainfoParser();
          Typo in classname above: missing 'n' between 'o' and 'P'

          Fixed the name everywhere

          + log.debug("commandissued: component: {} is in {}", componentName, currentState);
          Insert space between 'd' and 'i'.

          Added

          + if (pkg != null && !pkg.isEmpty()
          + && !pkg.equals(Component.MASTER_PACKAGE_NAME)) {
          The above can be simplified if you use Component.MASTER_PACKAGE_NAME.equals(pkg)

          when we are processing command for master package, pkg would be null or empty. that's why the code is written like this

          + public void applyCommandResult(CommandResult result, Command command, String pkg) {
          ...
          + public void applyCommandResult(CommandResult result, Command command) {
          Looks like there is common code between the above two methods which can be extracted.

          sounds good. Simplified

          + for (String pkg : pkgStatuses.keySet()) {
          + State pkgState = pkgStatuses.get(pkg);
          Please use pkgStatuses.entrySet() since both key and value are accessed.
          I think the two loops can be combined: if State.INSTALLING is encountered, return Command.NOP. Otherwise remember the pkg until the end of loop and return Command.INSTALL_ADDON for the pkg.

          Originally, I planned to code in the way you described. Saving the iterated pkg name while checking state in INSTALLING may be a little confusing. For better readability, shall we use two loops to explicitly show a higher rank on checking state in INSTALLING?

          + case INSTALL_ADDON:
          + if (this == State.INIT || this == State.INSTALL_FAILED) {
          + return State.INSTALLING;
          For State.INSTALL_FAILED, do we need to keep a counter to limit the number of failures ?

          The path is following what is kept for the master package currently. Let's open another JIRA to visit INSTALL FAILURE all together.

          +/**
          + *
          + */
          +public abstract class AbstractMetainfoParser {

          Sounds good. Added

          Show
          thomas_liu thomas liu added a comment - - edited Thank you for your comments. Here are my response to them: Some minor comments: + log.debug("metainfo map for master and addon is: {}", + packageMetainfo.toString()); The contents of packageMetainfo have been logged in the prior while loop. Is the above needed ? This one is used to make sure all packages' metainfo is read, which is not helped by the one in the loop. The one in the loop can be ignored. I'm removing it from the loop + log.debug("for comp " + roleName + " pkg " + componentStatus.getNextPkgToInstall() + " issuing " + command.toString()); Wrap long line. In HBase, line length is limited to 100. Sounds good. Wrapped. + ComponentCommand installCmd = null; + for (ComponentCommand compCmd : comp.getCommands()) { + if (compCmd.getName().equals("INSTALL")) { + installCmd = compCmd; + } + } + addInstallCommand(roleName, containerId, response, null, + installCmd, timeout, nextPkgToInstall); What if 'INSTALL' command isn't found in the for loop above ? I'm following the same pattern for master package. Please refer to "if (command == Command.INSTALL) "block above. Validation on the metainfo guarantees that there must be python script or INSTALL command + String appDef, boolean addonPackage) throws IOException, BadConfigException { Long line. Wrapped + * @param pkg TODO Please finish javadoc Done + metainfoParser = new AddoPackageMetainfoParser(); Typo in classname above: missing 'n' between 'o' and 'P' Fixed the name everywhere + log.debug("commandissued: component: {} is in {}", componentName, currentState); Insert space between 'd' and 'i'. Added + if (pkg != null && !pkg.isEmpty() + && !pkg.equals(Component.MASTER_PACKAGE_NAME)) { The above can be simplified if you use Component.MASTER_PACKAGE_NAME.equals(pkg) when we are processing command for master package, pkg would be null or empty. that's why the code is written like this + public void applyCommandResult(CommandResult result, Command command, String pkg) { ... + public void applyCommandResult(CommandResult result, Command command) { Looks like there is common code between the above two methods which can be extracted. sounds good. Simplified + for (String pkg : pkgStatuses.keySet()) { + State pkgState = pkgStatuses.get(pkg); Please use pkgStatuses.entrySet() since both key and value are accessed. I think the two loops can be combined: if State.INSTALLING is encountered, return Command.NOP. Otherwise remember the pkg until the end of loop and return Command.INSTALL_ADDON for the pkg. Originally, I planned to code in the way you described. Saving the iterated pkg name while checking state in INSTALLING may be a little confusing. For better readability, shall we use two loops to explicitly show a higher rank on checking state in INSTALLING? + case INSTALL_ADDON: + if (this == State.INIT || this == State.INSTALL_FAILED) { + return State.INSTALLING; For State.INSTALL_FAILED, do we need to keep a counter to limit the number of failures ? The path is following what is kept for the master package currently. Let's open another JIRA to visit INSTALL FAILURE all together. +/** + * + */ +public abstract class AbstractMetainfoParser { Sounds good. Added
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Suggested change which folds two loops in ComponentInstanceState#getNextCommand() into one.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Suggested change which folds two loops in ComponentInstanceState#getNextCommand() into one.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 4197c8bb8aef4490c0c4994bd8d22292b4b93dfe in incubator-slider's branch refs/heads/develop from Sumit Mohanty
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=4197c8b ]

          SLIDER-773. Add co-processor support for app packages (adding missing licence headers)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 4197c8bb8aef4490c0c4994bd8d22292b4b93dfe in incubator-slider's branch refs/heads/develop from Sumit Mohanty [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=4197c8b ] SLIDER-773 . Add co-processor support for app packages (adding missing licence headers)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 016cd66aee97fe9752f78f9f8eb1283c162cc4d3 in incubator-slider's branch refs/heads/develop from Sumit Mohanty
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=016cd66 ]

          SLIDER-773. Add co-processor support for app packages (Thomas Liu via smohanty)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 016cd66aee97fe9752f78f9f8eb1283c162cc4d3 in incubator-slider's branch refs/heads/develop from Sumit Mohanty [ https://git-wip-us.apache.org/repos/asf?p=incubator-slider.git;h=016cd66 ] SLIDER-773 . Add co-processor support for app packages (Thomas Liu via smohanty)
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Some minor comments:

          +            log.debug("metainfo map for master and addon is: {}",
          +                packageMetainfo.toString());
          

          The contents of packageMetainfo have been logged in the prior while loop. Is the above needed ?

          +    log.debug("for comp " + roleName + " pkg " + componentStatus.getNextPkgToInstall() + " issuing " + command.toString());
          

          Wrap long line. In HBase, line length is limited to 100.

          +                ComponentCommand installCmd = null;
          +                for (ComponentCommand compCmd : comp.getCommands()) {
          +                  if (compCmd.getName().equals("INSTALL")) {
          +                    installCmd = compCmd;
          +                  }
          +                }
          +                addInstallCommand(roleName, containerId, response, null,
          +                    installCmd, timeout, nextPkgToInstall);
          

          What if 'INSTALL' command isn't found in the for loop above ?

          +                                            String appDef, boolean addonPackage) throws IOException, BadConfigException {
          

          Long line.

          +   * @param pkg TODO
          

          Please finish javadoc

          +      metainfoParser = new AddoPackageMetainfoParser();
          

          Typo in classname above: missing 'n' between 'o' and 'P'

          +      log.debug("commandissued: component: {} is in {}", componentName, currentState);
          

          Insert space between 'd' and 'i'.

          +    if (pkg != null && !pkg.isEmpty()
          +        && !pkg.equals(Component.MASTER_PACKAGE_NAME)) {
          

          The above can be simplified if you use Component.MASTER_PACKAGE_NAME.equals(pkg)

          +  public void applyCommandResult(CommandResult result, Command command, String pkg) {
          ...
          +  public void applyCommandResult(CommandResult result, Command command) {
          

          Looks like there is common code between the above two methods which can be extracted.

          +      for (String pkg : pkgStatuses.keySet()) {
          +        State pkgState = pkgStatuses.get(pkg);
          

          Please use pkgStatuses.entrySet() since both key and value are accessed.
          I think the two loops can be combined: if State.INSTALLING is encountered, return Command.NOP. Otherwise remember the pkg until the end of loop and return Command.INSTALL_ADDON for the pkg.

          +      case INSTALL_ADDON:
          +          if (this == State.INIT || this == State.INSTALL_FAILED) {
          +            return State.INSTALLING;
          

          For State.INSTALL_FAILED, do we need to keep a counter to limit the number of failures ?

          +/**
          + *
          + */
          +public abstract class AbstractMetainfoParser {
          

          Please fill in javadoc for the class.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Some minor comments: + log.debug( "metainfo map for master and addon is: {}" , + packageMetainfo.toString()); The contents of packageMetainfo have been logged in the prior while loop. Is the above needed ? + log.debug( " for comp " + roleName + " pkg " + componentStatus.getNextPkgToInstall() + " issuing " + command.toString()); Wrap long line. In HBase, line length is limited to 100. + ComponentCommand installCmd = null ; + for (ComponentCommand compCmd : comp.getCommands()) { + if (compCmd.getName().equals( "INSTALL" )) { + installCmd = compCmd; + } + } + addInstallCommand(roleName, containerId, response, null , + installCmd, timeout, nextPkgToInstall); What if 'INSTALL' command isn't found in the for loop above ? + String appDef, boolean addonPackage) throws IOException, BadConfigException { Long line. + * @param pkg TODO Please finish javadoc + metainfoParser = new AddoPackageMetainfoParser(); Typo in classname above: missing 'n' between 'o' and 'P' + log.debug( "commandissued: component: {} is in {}" , componentName, currentState); Insert space between 'd' and 'i'. + if (pkg != null && !pkg.isEmpty() + && !pkg.equals(Component.MASTER_PACKAGE_NAME)) { The above can be simplified if you use Component.MASTER_PACKAGE_NAME.equals(pkg) + public void applyCommandResult(CommandResult result, Command command, String pkg) { ... + public void applyCommandResult(CommandResult result, Command command) { Looks like there is common code between the above two methods which can be extracted. + for ( String pkg : pkgStatuses.keySet()) { + State pkgState = pkgStatuses.get(pkg); Please use pkgStatuses.entrySet() since both key and value are accessed. I think the two loops can be combined: if State.INSTALLING is encountered, return Command.NOP. Otherwise remember the pkg until the end of loop and return Command.INSTALL_ADDON for the pkg. + case INSTALL_ADDON: + if ( this == State.INIT || this == State.INSTALL_FAILED) { + return State.INSTALLING; For State.INSTALL_FAILED, do we need to keep a counter to limit the number of failures ? +/** + * + */ + public abstract class AbstractMetainfoParser { Please fill in javadoc for the class.
          Hide
          thomas_liu thomas liu added a comment -

          Merged Gour's latest change

          Show
          thomas_liu thomas liu added a comment - Merged Gour's latest change
          Hide
          thomas_liu thomas liu added a comment -

          Modify the code according to Steve's comments.
          Will check in code changes of test cases later

          Show
          thomas_liu thomas liu added a comment - Modify the code according to Steve's comments. Will check in code changes of test cases later
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I should warn that I that I don't know the .py-side of things (or the agent provider) as well as Gour or Sumit; their reviews are needed too. From what I can see, looks good

          .h2 Production code

          .h3 Heartbeat.py

          -what if there is no componentPackage? will it be null or ""? And does the AM expect this

          .h3 AgentProviderService.java

          like 764, looks like a log entry was accidentally deleted ... can you restore it.

          line 1679; the javadocs have a @param pkg, but the method doesn't. Is this an incomplete method or just a javadoc line to be deleted?

          .h3 State.java

          class needs a toString method printing state, if its to be used in exception messages.

          .h3 AbstractComponent

          fields should be protected or private

          .h3 AbstractMetainfoParser

          I see that when there are parsing problems, the exceptions are simply caught and null returned. This is exactly what MetainfoParser. I don't think it should be done. The exceptions should be logged @ debug level, to help debug things like XML parse problems. (Ultimately that exception should be relayed -this could be filed as a separate JIRA)

          .h3 logging

          • logs (especially debug ones), should use the Slf4J built in expansion, of things like
          • I think there's too much @ info, such as in ComponentInstanceState. They can be downgraded to debug; if needed for diagnosing things, log4j.properties is easy to edit. That
            log.debug("addonMap.get(key): {}" , addonMap.get(key))
            

          it is a bit more efficient as it doesn't do the string expansion unless needed, and reports nulls too.

          Tests

          I think we need more tests.

          1. Building a cluster: add tests to build a cluster with a package; include ones to test invalid details (e.g. no such package, package with bad metadata)
          2. copy of TestAgentEcho with a stub package
          3. Anything we can do to test CLI

          Functional tests can be added as a separate/sub JIRA; I can imagine two

          1. add a simple package to one of the AgentIT tests; have it do something minimal like touch a file in HDFS. The test would look for that file existing.
          2. add a missing stub package, expect failures

          Documentation

          needs a JIRA on this

          Show
          stevel@apache.org Steve Loughran added a comment - I should warn that I that I don't know the .py-side of things (or the agent provider) as well as Gour or Sumit; their reviews are needed too. From what I can see, looks good .h2 Production code .h3 Heartbeat.py -what if there is no componentPackage? will it be null or ""? And does the AM expect this .h3 AgentProviderService.java like 764, looks like a log entry was accidentally deleted ... can you restore it. line 1679; the javadocs have a @param pkg , but the method doesn't. Is this an incomplete method or just a javadoc line to be deleted? .h3 State.java class needs a toString method printing state, if its to be used in exception messages. .h3 AbstractComponent fields should be protected or private .h3 AbstractMetainfoParser I see that when there are parsing problems, the exceptions are simply caught and null returned. This is exactly what MetainfoParser . I don't think it should be done. The exceptions should be logged @ debug level, to help debug things like XML parse problems. (Ultimately that exception should be relayed -this could be filed as a separate JIRA) .h3 logging logs (especially debug ones), should use the Slf4J built in expansion, of things like I think there's too much @ info, such as in ComponentInstanceState. They can be downgraded to debug; if needed for diagnosing things, log4j.properties is easy to edit. That log.debug( "addonMap.get(key): {}" , addonMap.get(key)) it is a bit more efficient as it doesn't do the string expansion unless needed, and reports nulls too. Tests I think we need more tests. Building a cluster: add tests to build a cluster with a package; include ones to test invalid details (e.g. no such package, package with bad metadata) copy of TestAgentEcho with a stub package Anything we can do to test CLI Functional tests can be added as a separate/sub JIRA; I can imagine two add a simple package to one of the AgentIT tests; have it do something minimal like touch a file in HDFS. The test would look for that file existing. add a missing stub package, expect failures Documentation needs a JIRA on this
          Hide
          thomas_liu thomas liu added a comment - - edited

          How to package and submit add on packages:
          When users would like to submit an application composed of master package and add on packages, they need to provide the master package(the existing application package) without modification as it is, and also the add on packages. The addon packages can be created the same way as master package. For submission, they still use 'slider create' command, with additional parameters: --addon <package name> <package zip file location in local FS>. The detail of packaging, especially the schema of appConfig.json, metainfo.json are defined in https://issues.apache.org/jira/browse/SLIDER-834

          Current design:
          We are not modifying the basic architecture of Slider, either how SliderAgent communicate with SliderAM. We modify the state machine in AgentProviderService to issue INSTALL commands for add on packages. Details here: https://issues.apache.org/jira/browse/SLIDER-820

          Show
          thomas_liu thomas liu added a comment - - edited How to package and submit add on packages: When users would like to submit an application composed of master package and add on packages, they need to provide the master package(the existing application package) without modification as it is, and also the add on packages. The addon packages can be created the same way as master package. For submission, they still use 'slider create' command, with additional parameters: --addon <package name> <package zip file location in local FS>. The detail of packaging, especially the schema of appConfig.json, metainfo.json are defined in https://issues.apache.org/jira/browse/SLIDER-834 Current design: We are not modifying the basic architecture of Slider, either how SliderAgent communicate with SliderAM. We modify the state machine in AgentProviderService to issue INSTALL commands for add on packages. Details here: https://issues.apache.org/jira/browse/SLIDER-820
          Hide
          sumitmohanty Sumit Mohanty added a comment -

          In general, I am looking at the problem of influencing a slider app package by adding layers that can modify the file layout, add new config files, or even modify configs in existing files. You can think of calling "INSTALL" or "CONFIG" on all add-ons after you have called the same commands for the original package.

          The most common example is likely the problem addressed by SLIDER-757 where additional jars are being added to the class path. There is also a feature where you can drop additional config files that will be auto-copied to the config folder but the expectation is that the config files are fully specified (I need to refersh my memory as to how).

          SLIDER-773 will be looking at a general purpose approach that help get around the limitation that an admin cannot go to a specific host, influence lib dir, conf dir, etc. when apps are hosted on YARN. Once we have a proposal we can see if it can effectively subsume the need for additional_resource_dir.

          Another aspect that I am exploring is to not require a .zip file but just specify a structured folder that may have a metainfo.xml and some files. The basic form of that is providing a directory where all files in the directory are copied and made available to AM and container instances.

          I will share the proposal on this JIRA soon.

          Show
          sumitmohanty Sumit Mohanty added a comment - In general, I am looking at the problem of influencing a slider app package by adding layers that can modify the file layout, add new config files, or even modify configs in existing files. You can think of calling "INSTALL" or "CONFIG" on all add-ons after you have called the same commands for the original package. The most common example is likely the problem addressed by SLIDER-757 where additional jars are being added to the class path. There is also a feature where you can drop additional config files that will be auto-copied to the config folder but the expectation is that the config files are fully specified ( I need to refersh my memory as to how ). SLIDER-773 will be looking at a general purpose approach that help get around the limitation that an admin cannot go to a specific host, influence lib dir, conf dir, etc. when apps are hosted on YARN. Once we have a proposal we can see if it can effectively subsume the need for additional_resource_dir . Another aspect that I am exploring is to not require a .zip file but just specify a structured folder that may have a metainfo.xml and some files. The basic form of that is providing a directory where all files in the directory are copied and made available to AM and container instances. I will share the proposal on this JIRA soon.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Sumit -is this a duplicate of SLIDER-757? The use case "add additional resources" is.

          Show
          stevel@apache.org Steve Loughran added a comment - Sumit -is this a duplicate of SLIDER-757 ? The use case "add additional resources" is.

            People

            • Assignee:
              thomas_liu thomas liu
              Reporter:
              sumitmohanty Sumit Mohanty
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile