IGNITE-4052 ready for review - what can I improve

[vadopolski]

Hello community!

I want to add ability to connect to MESOS cluster via user name and role
from system env properties and to add JUnit test.

Review please pull request, what can I improve ? How correctly to test
methods work?

https://github.com/apache/ignite/pull/1662

Vadim Opolski


2017-03-30 16:32 GMT+03:00 Вадим Опольский <[hidden email]>:

> Hello everyone!
>
> Nikolay, method Protos.FrameworkInfo.Builder#setRoleBytes and
> Protos.FrameworkInfo.Builder#setUserBytes were added into
> IgniteFrameworkInfoTest.
>
> Details please what do you want to me do with the methods. How
> correctly to test methods work?
>
> The code was changed with official "Coding Guidelines" and the lib was
> deleted.
>
> I didn't try to use new properties on real mesos cluster.
>
> https://github.com/apache/ignite/pull/1662
>
> Vadim Opolski
>
> *You used only Protos.FrameworkInfo.Builder#setRole method, but also
> exists Protos.FrameworkInfo.Builder#setRoleBytes. Please, pay attention on
> it. You did some code styles changes which conflict with official "Coding
> Guidelines" (see *
> *https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute*
> <https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute>*).
> Also you use lib for testing with licence different from Apache Licence
> 2.0. Are you sure that for this test you need additional libs? I think this
> can be implemented without them.*
>
> *Also did you try to use new properties on real mesos cluster? Did work
> properly?*
>
> 2017-03-24 15:54 GMT+03:00 Вадим Опольский <[hidden email]>:
>
>> Nikolay, I will add properties for mesos role and unit test next week.
>>
>> ---------- Forwarded message ----------
>> From: Вадим Опольский <[hidden email]>
>> Date: 2017-03-22 15:53 GMT+03:00
>> Subject: Re: IGNITE-4052 ready for review
>> To: [hidden email]
>>
>>
>> Nikolay, just changed status to "path available".
>>
>> 2017-03-22 15:44 GMT+03:00 Nikolai Tikhonov <[hidden email]>:
>>
>>> Hi Вадим!
>>>
>>> Thank you for your contribution!
>>> Please change status of the ticket to "path available". I'll review your
>>> changes.
>>>
>>> Thanks,
>>> Nikolay
>>>
>>> On Wed, Mar 22, 2017 at 3:36 PM, Вадим Опольский <[hidden email]>
>>> wrote:
>>>
>>> > Hello everybody!
>>> >
>>> > Nikolay,
>>> >  review please https://github.com/apache/ignite/pull/1662 .
>>> >
>>> > Added ability to configure current user parameters via system env
>>> > properties - "MESOS_USER".
>>> >
>>> > Vadim Opolski
>>> >
>>> >
>>> > ---------- Forwarded message ----------
>>> > From: Вадим Опольский <[hidden email]>
>>> > Date: 2017-03-21 14:40 GMT+03:00
>>> > Subject: Assignee IGNITE-4052
>>> > To: [hidden email]
>>> >
>>> >
>>> > Dear sirs !
>>> >
>>> > I want to resolve issue IGNITE-4052.
>>> >
>>> > https://issues.apache.org/jira/browse/IGNITE-4052
>>> >
>>> > Is it actual ?
>>> >
>>> > Vadim Opolski
>>> >
>>> >
>>>
>>
>>
>>
>

[dmagda]

Nick, Vadim,

Please don’t forget to update Mesos doc once this contribution gets merged:
https://apacheignite.readme.io/docs/mesos-deployment <https://apacheignite.readme.io/docs/mesos-deployment>

—
Denis

> On Mar 31, 2017, at 4:07 AM, Вадим Опольский <[hidden email]> wrote:
>
> Hello community!
>
> I want to add ability to connect to MESOS cluster via user name and role
> from system env properties and to add JUnit test.
>
> Review please pull request, what can I improve ? How correctly to test
> methods work?
>
> https://github.com/apache/ignite/pull/1662
>
> Vadim Opolski
>
>
> 2017-03-30 16:32 GMT+03:00 Вадим Опольский <[hidden email]>:
>
>> Hello everyone!
>>
>> Nikolay, method Protos.FrameworkInfo.Builder#setRoleBytes and
>> Protos.FrameworkInfo.Builder#setUserBytes were added into
>> IgniteFrameworkInfoTest.
>>
>> Details please what do you want to me do with the methods. How
>> correctly to test methods work?
>>
>> The code was changed with official "Coding Guidelines" and the lib was
>> deleted.
>>
>> I didn't try to use new properties on real mesos cluster.
>>
>> https://github.com/apache/ignite/pull/1662
>>
>> Vadim Opolski
>>
>> *You used only Protos.FrameworkInfo.Builder#setRole method, but also
>> exists Protos.FrameworkInfo.Builder#setRoleBytes. Please, pay attention on
>> it. You did some code styles changes which conflict with official "Coding
>> Guidelines" (see *
>> *https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute*
>> <https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute>*).
>> Also you use lib for testing with licence different from Apache Licence
>> 2.0. Are you sure that for this test you need additional libs? I think this
>> can be implemented without them.*
>>
>> *Also did you try to use new properties on real mesos cluster? Did work
>> properly?*
>>
>> 2017-03-24 15:54 GMT+03:00 Вадим Опольский <[hidden email]>:
>>
>>> Nikolay, I will add properties for mesos role and unit test next week.
>>>
>>> ---------- Forwarded message ----------
>>> From: Вадим Опольский <[hidden email]>
>>> Date: 2017-03-22 15:53 GMT+03:00
>>> Subject: Re: IGNITE-4052 ready for review
>>> To: [hidden email]
>>>
>>>
>>> Nikolay, just changed status to "path available".
>>>
>>> 2017-03-22 15:44 GMT+03:00 Nikolai Tikhonov <[hidden email]>:
>>>
>>>> Hi Вадим!
>>>>
>>>> Thank you for your contribution!
>>>> Please change status of the ticket to "path available". I'll review your
>>>> changes.
>>>>
>>>> Thanks,
>>>> Nikolay
>>>>
>>>> On Wed, Mar 22, 2017 at 3:36 PM, Вадим Опольский <[hidden email]>
>>>> wrote:
>>>>
>>>>> Hello everybody!
>>>>>
>>>>> Nikolay,
>>>>> review please https://github.com/apache/ignite/pull/1662 .
>>>>>
>>>>> Added ability to configure current user parameters via system env
>>>>> properties - "MESOS_USER".
>>>>>
>>>>> Vadim Opolski
>>>>>
>>>>>
>>>>> ---------- Forwarded message ----------
>>>>> From: Вадим Опольский <[hidden email]>
>>>>> Date: 2017-03-21 14:40 GMT+03:00
>>>>> Subject: Assignee IGNITE-4052
>>>>> To: [hidden email]
>>>>>
>>>>>
>>>>> Dear sirs !
>>>>>
>>>>> I want to resolve issue IGNITE-4052.
>>>>>
>>>>> https://issues.apache.org/jira/browse/IGNITE-4052
>>>>>
>>>>> Is it actual ?
>>>>>
>>>>> Vadim Opolski
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>

[Nikolai Tikhonov]

Sure!

On Fri, Mar 31, 2017 at 8:13 PM, Denis Magda <[hidden email]> wrote:

> Nick, Vadim,
>
> Please don’t forget to update Mesos doc once this contribution gets merged:
> https://apacheignite.readme.io/docs/mesos-deployment
>
> —
> Denis
>
> On Mar 31, 2017, at 4:07 AM, Вадим Опольский <[hidden email]> wrote:
>
> Hello community!
>
> I want to add ability to connect to MESOS cluster via user name and role
> from system env properties and to add JUnit test.
>
> Review please pull request, what can I improve ? How correctly to test
> methods work?
>
> https://github.com/apache/ignite/pull/1662
>
> Vadim Opolski
>
>
> 2017-03-30 16:32 GMT+03:00 Вадим Опольский <[hidden email]>:
>
> Hello everyone!
>
> Nikolay, method Protos.FrameworkInfo.Builder#setRoleBytes and
> Protos.FrameworkInfo.Builder#setUserBytes were added into
> IgniteFrameworkInfoTest.
>
> Details please what do you want to me do with the methods. How
> correctly to test methods work?
>
> The code was changed with official "Coding Guidelines" and the lib was
> deleted.
>
> I didn't try to use new properties on real mesos cluster.
>
> https://github.com/apache/ignite/pull/1662
>
> Vadim Opolski
>
> *You used only Protos.FrameworkInfo.Builder#setRole method, but also
> exists Protos.FrameworkInfo.Builder#setRoleBytes. Please, pay attention on
> it. You did some code styles changes which conflict with official "Coding
> Guidelines" (see *
> *https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute*
> <https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute>*).
> Also you use lib for testing with licence different from Apache Licence
> 2.0. Are you sure that for this test you need additional libs? I think this
> can be implemented without them.*
>
> *Also did you try to use new properties on real mesos cluster? Did work
> properly?*
>
>
> 2017-03-24 15:54 GMT+03:00 Вадим Опольский <[hidden email]>:
>
> Nikolay, I will add properties for mesos role and unit test next week.
>
> ---------- Forwarded message ----------
> From: Вадим Опольский <[hidden email]>
> Date: 2017-03-22 15:53 GMT+03:00
> Subject: Re: IGNITE-4052 ready for review
> To: [hidden email]
>
>
> Nikolay, just changed status to "path available".
>
> 2017-03-22 15:44 GMT+03:00 Nikolai Tikhonov <[hidden email]>:
>
> Hi Вадим!
>
> Thank you for your contribution!
> Please change status of the ticket to "path available". I'll review your
> changes.
>
> Thanks,
> Nikolay
>
> On Wed, Mar 22, 2017 at 3:36 PM, Вадим Опольский <[hidden email]>
> wrote:
>
> Hello everybody!
>
> Nikolay,
> review please https://github.com/apache/ignite/pull/1662 .
>
> Added ability to configure current user parameters via system env
> properties - "MESOS_USER".
>
> Vadim Opolski
>
>
> ---------- Forwarded message ----------
> From: Вадим Опольский <[hidden email]>
> Date: 2017-03-21 14:40 GMT+03:00
> Subject: Assignee IGNITE-4052
> To: [hidden email]
>
>
> Dear sirs !
>
> I want to resolve issue IGNITE-4052.
>
> https://issues.apache.org/jira/browse/IGNITE-4052
>
> Is it actual ?
>
> Vadim Opolski
>
>
>
>
>
>
>
>
>