[
https://issues.apache.org/jira/browse/SLIDER-773?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14491879#comment-14491879
]
thomas liu commented on SLIDER-773:
-----------------------------------
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)
sounds good. Simplified
+ 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
> Add co-processor support for app packages
> -----------------------------------------
>
> Key: SLIDER-773
> URL: https://issues.apache.org/jira/browse/SLIDER-773
> Project: Slider
> Issue Type: Bug
> Components: app-package, client
> Affects Versions: Slider 0.60
> Reporter: Sumit Mohanty
> Assignee: thomas liu
> Priority: Critical
> Fix For: Slider 0.80
>
> Attachments: 773-suggest.txt, Co-processorSupport.pdf,
> coprocessor-apri-4th.patch, coprocessor.patch, coprocessor.patch
>
>
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
