[
https://issues.apache.org/jira/browse/SLIDER-773?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14498514#comment-14498514
]
thomas liu commented on SLIDER-773:
-----------------------------------
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) {
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
Those are overriding, given that the children class's variable type has changed
Do not checkin code with default stub codes like // TODO Auto-generated method
stub
> 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,
> SLIDER-773_review_comments.patch, coprocessor-after.patch,
> 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)
