Mailing-List: contact issues-help@ignite.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@ignite.incubator.apache.org
Date: Fri, 5 Jun 2015 11:14:00 +0000 (UTC)
From: "Artem Shutak (JIRA)" <jira@apache.org>
To: issues@ignite.incubator.apache.org
Message-ID: <JIRA.12830666.1431952452000.1464.1433502840382@Atlassian.JIRA>
In-Reply-To: <JIRA.12830666.1431952452000@Atlassian.JIRA>
References: <JIRA.12830666.1431952452000@Atlassian.JIRA>
 <JIRA.12830666.1431952452367@arcas>
Subject: [jira] [Comment Edited] (IGNITE-917) Add
 org.apache.ignite.cluster.ClusterGroup.forHost(String host, String...
 hosts) overload
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ 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)