[
https://issues.apache.org/jira/browse/SLIDER-773?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14495617#comment-14495617
]
Gour Saha commented on SLIDER-773:
----------------------------------
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.
{code}
} catch (SliderException e) {
log.warn("Component instance failed operation.", e);
componentStatus.applyCommandResult(CommandResult.FAILED, command, null);
}
{code}
# 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?
{code}
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);
{code}
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 [[email protected]] already mentioned, avoid long lines (wrap beyond 80
chars)
# As [[email protected]] 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 -
{code}
TreeMap<String, State> pkgStatuses = new TreeMap<String, State>();
{code}
instead use
{code}
Map<String, State> pkgStatuses = new TreeMap<String, State>();
{code}
# As [[email protected]] mentioned
{{Component.MASTER_PACKAGE_NAME.equals(pkg)}} covers the check for
{{pkg.isEmpty()}}
# In {{ComponentInstanceState.java}} as [[email protected]] 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)?
{code}
nextPkgToInstall = null;
{code}
# 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}}
> 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)
