[
https://issues.apache.org/jira/browse/IGNITE-917?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14574327#comment-14574327
]
Artem Shutak edited comment on IGNITE-917 at 6/5/15 11:13 AM:
--------------------------------------------------------------
Hi Atri,
1. I can understand, why you have problems with indention (change tabulation
indention with 4 spaces is not so easy). Can you, please, at lease remove
redundant empty line. Like here (see another places too)
{code}
private static class HostsFilter implements IgnitePredicate<ClusterNode> {
/** */
private static final long serialVersionUID = 0L;
{code}
and here
{code}
private HostsFilter(String name, String[] names) {
this.inputHostNames = new ArrayList<String> (Arrays.asList(names));
{code}
About bracing see
https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-BracketsandIdentation.
It's a wrong javadoc (1 @param instead of 2)
{code}
/**
* @param names Names.
*/
private HostsFilter(String name, String[] names) {
{code}
{code} CurrentHostName{code} - bad variable naming.
Please, be really careful with Coding Guidelines. I think we will not be able
to finish review before there are issues here.
2. Small points
- Redundant "this" here {code}this.inputHostNames{code}
- Here {code}new ArrayList<String>{code} "String" is redundant and can be
removed (Ignite supports only java 7 and higher).
3. Why do you wrap all objects in ArrayList?
For example, {code}Arrays.asList(names){code} already returns ArrayList.
Are you really need to have n.hostNames() as List? I think no.
Actually, only this place is really important:
{code}inputHostNames.contains(CurrentHostName){code}
Implementation of ArrayList.contains() is not too fast. I suggest to use
HashSet here.
4. It was my mistake, but I forget to point you about new test for new public
method. I think you should add it at GridProjectionSelfTest.
was (Author: ashutak):
Hi Atri,
1. I can understand, why you have problems with indention (change tabulation
indention with 4 spaces is not so easy). Can you, please, at lease remove
redundant empty line. Like here (see another places too)
{code}
private static class HostsFilter implements IgnitePredicate<ClusterNode> {
/** */
private static final long serialVersionUID = 0L;
{code}
and here
{code}
private HostsFilter(String name, String[] names) {
this.inputHostNames = new ArrayList<String> (Arrays.asList(names));
{code}
About bracing see
https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-BracketsandIdentation.
It's a wrong javadoc (1 @param instead of 2)
{code}
/**
* @param names Names.
*/
private HostsFilter(String name, String[] names) {
{code}
{code} CurrentHostName - bad variable naming.
Please, be really careful with Coding Guidelines. I think we will not be able
to finish review before there are issues here.
2. Small points
- Redundant "this" here {code}this.inputHostNames{code}
- Here {code}new ArrayList<String>{code} "String" is redundant and can be
removed (Ignite supports only java 7 and higher).
3. Why do you wrap all objects in ArrayList?
For example, {code}Arrays.asList(names){code} already returns ArrayList.
Are you really need to have n.hostNames() as List? I think no.
Actually, only this place is really important:
{code}inputHostNames.contains(CurrentHostName){code}
Implementation of ArrayList.contains() is not too fast. I suggest to use
HashSet here.
4. It was my mistake, but I forget to point you about new test for new public
method. I think you should add it at GridProjectionSelfTest.
> Add org.apache.ignite.cluster.ClusterGroup.forHost(String host, String...
> hosts) overload
> -----------------------------------------------------------------------------------------
>
> Key: IGNITE-917
> URL: https://issues.apache.org/jira/browse/IGNITE-917
> Project: Ignite
> Issue Type: Task
> Components: newbie
> Reporter: Yakov Zhdanov
> Assignee: Yakov Zhdanov
> Attachments: ignite-sprint-5_ignite-917ver2.patch
>
>
> Method should build a cluster group from all nodes running on the hosts
> specified.
> This should be dynamic group. I think method implementation should build up a
> predicate to check that passed in node's host names contain one of the host
> names from parameters.
> See IgnitePredicate, ClusterGroup, ClusterNode, ClusterNode#hostNames
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
