IGNITE-5123 Review

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

IGNITE-5123 Review

Evgeniy Ignatiev
Hello, Igniters.

Could anyone review my request -
https://issues.apache.org/jira/browse/IGNITE-5123? - My previous pings
seems to got lost.

Best regards,

Yevgeniy

Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Anton Vinogradov-2
Igniters,

Could somebody review the fix today?

On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
[hidden email]> wrote:

> Hello, Igniters.
>
> Could anyone review my request - https://issues.apache.org/jira
> /browse/IGNITE-5123? - My previous pings seems to got lost.
>
> Best regards,
>
> Yevgeniy
>
>
Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Dmitriy Pavlov
Hi Evgeniy,

I was not able to find Teamcity run for this change.
Could you please run http://ci.ignite.apache.org test for example on branch
pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
Or could you please share link to previous run on this changes?

Sincerely,
Dmitriy Pavlov

ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:

> Igniters,
>
> Could somebody review the fix today?
>
> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
> [hidden email]> wrote:
>
> > Hello, Igniters.
> >
> > Could anyone review my request - https://issues.apache.org/jira
> > /browse/IGNITE-5123? - My previous pings seems to got lost.
> >
> > Best regards,
> >
> > Yevgeniy
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Evgeniy Ignatiev
http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic 
- this one - there seem to be no new failed platform tests, other failed
tests seem to fail in several other reviews too and are unrelated to my
changes.


On 19.07.2017 17:35, Dmitry Pavlov wrote:

> Hi Evgeniy,
>
> I was not able to find Teamcity run for this change.
> Could you please run http://ci.ignite.apache.org test for example on branch
> pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
> Or could you please share link to previous run on this changes?
>
> Sincerely,
> Dmitriy Pavlov
>
> ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
>
>> Igniters,
>>
>> Could somebody review the fix today?
>>
>> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
>> [hidden email]> wrote:
>>
>>> Hello, Igniters.
>>>
>>> Could anyone review my request - https://issues.apache.org/jira
>>> /browse/IGNITE-5123? - My previous pings seems to got lost.
>>>
>>> Best regards,
>>>
>>> Yevgeniy
>>>
>>>

Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Dmitriy Pavlov
Hi Evgeniy,

Thank you. Ignite Basic is one suite from approximately 80 suites that
covers Ignite by automated tests. Which is why I suggested to use RunAll
chain in ignite 2.0 group. Yes, several tests may fail, especially if it is
flaky tests or failure is related to the specific JIRA issue.

About change itself: This change seems to be very impact. There is
possiblity that many of existing plugins relies on existing order of
initialization. This change may break plugin initialization in unexpected
manner.

Could we
- fix javadoc according to existing order in code
- find out new solution?

Sincerely,
Dmitriy Pavlov


ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev <[hidden email]>:

>
> http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
> - this one - there seem to be no new failed platform tests, other failed
> tests seem to fail in several other reviews too and are unrelated to my
> changes.
>
>
> On 19.07.2017 17:35, Dmitry Pavlov wrote:
> > Hi Evgeniy,
> >
> > I was not able to find Teamcity run for this change.
> > Could you please run http://ci.ignite.apache.org test for example on
> branch
> > pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
> > Or could you please share link to previous run on this changes?
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
> >
> >> Igniters,
> >>
> >> Could somebody review the fix today?
> >>
> >> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
> >> [hidden email]> wrote:
> >>
> >>> Hello, Igniters.
> >>>
> >>> Could anyone review my request - https://issues.apache.org/jira
> >>> /browse/IGNITE-5123? - My previous pings seems to got lost.
> >>>
> >>> Best regards,
> >>>
> >>> Yevgeniy
> >>>
> >>>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

npordash
Hi Dmitriy,

The ticket was a regression from 1.9 to 2.0. I don't think anyone would be
expecting the behavior in 2.0 as it doesn't align with the javadoc and has
only been broken since the 2.0 release.

-Nick

On Wed, Jul 19, 2017, 6:55 AM Dmitry Pavlov <[hidden email]> wrote:

> Hi Evgeniy,
>
> Thank you. Ignite Basic is one suite from approximately 80 suites that
> covers Ignite by automated tests. Which is why I suggested to use RunAll
> chain in ignite 2.0 group. Yes, several tests may fail, especially if it is
> flaky tests or failure is related to the specific JIRA issue.
>
> About change itself: This change seems to be very impact. There is
> possiblity that many of existing plugins relies on existing order of
> initialization. This change may break plugin initialization in unexpected
> manner.
>
> Could we
> - fix javadoc according to existing order in code
> - find out new solution?
>
> Sincerely,
> Dmitriy Pavlov
>
>
> ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev <[hidden email]
> >:
>
> >
> >
> http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
> > - this one - there seem to be no new failed platform tests, other failed
> > tests seem to fail in several other reviews too and are unrelated to my
> > changes.
> >
> >
> > On 19.07.2017 17:35, Dmitry Pavlov wrote:
> > > Hi Evgeniy,
> > >
> > > I was not able to find Teamcity run for this change.
> > > Could you please run http://ci.ignite.apache.org test for example on
> > branch
> > > pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
> > > Or could you please share link to previous run on this changes?
> > >
> > > Sincerely,
> > > Dmitriy Pavlov
> > >
> > > ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
> > >
> > >> Igniters,
> > >>
> > >> Could somebody review the fix today?
> > >>
> > >> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
> > >> [hidden email]> wrote:
> > >>
> > >>> Hello, Igniters.
> > >>>
> > >>> Could anyone review my request - https://issues.apache.org/jira
> > >>> /browse/IGNITE-5123? - My previous pings seems to got lost.
> > >>>
> > >>> Best regards,
> > >>>
> > >>> Yevgeniy
> > >>>
> > >>>
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Evgeniy Ignatiev
In reply to this post by Dmitriy Pavlov
Scheduled "Run All" in Ignite 2.0 Tests for pull/2285/head -
http://ci.ignite.apache.org/viewQueued.html?itemId=734208


On 19.07.2017 17:55, Dmitry Pavlov wrote:

> Hi Evgeniy,
>
> Thank you. Ignite Basic is one suite from approximately 80 suites that
> covers Ignite by automated tests. Which is why I suggested to use RunAll
> chain in ignite 2.0 group. Yes, several tests may fail, especially if it is
> flaky tests or failure is related to the specific JIRA issue.
>
> About change itself: This change seems to be very impact. There is
> possiblity that many of existing plugins relies on existing order of
> initialization. This change may break plugin initialization in unexpected
> manner.
>
> Could we
> - fix javadoc according to existing order in code
> - find out new solution?
>
> Sincerely,
> Dmitriy Pavlov
>
>
> ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev <[hidden email]>:
>
>> http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
>> - this one - there seem to be no new failed platform tests, other failed
>> tests seem to fail in several other reviews too and are unrelated to my
>> changes.
>>
>>
>> On 19.07.2017 17:35, Dmitry Pavlov wrote:
>>> Hi Evgeniy,
>>>
>>> I was not able to find Teamcity run for this change.
>>> Could you please run http://ci.ignite.apache.org test for example on
>> branch
>>> pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
>>> Or could you please share link to previous run on this changes?
>>>
>>> Sincerely,
>>> Dmitriy Pavlov
>>>
>>> ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
>>>
>>>> Igniters,
>>>>
>>>> Could somebody review the fix today?
>>>>
>>>> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
>>>> [hidden email]> wrote:
>>>>
>>>>> Hello, Igniters.
>>>>>
>>>>> Could anyone review my request - https://issues.apache.org/jira
>>>>> /browse/IGNITE-5123? - My previous pings seems to got lost.
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Yevgeniy
>>>>>
>>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Evgeniy Ignatiev
Rerun tests in "Run All" Ignite 2.0 group after merge with the latest
master -
http://ci.ignite.apache.org/viewLog.html?buildId=734705&tab=queuedBuildOverviewTab 
- Do you see any issues with them? (Unfortunately I have too little
experience with Ignite tests to judge.)


On 7/19/2017 6:25 PM, Evgeniy Ignatiev wrote:

> Scheduled "Run All" in Ignite 2.0 Tests for pull/2285/head -
> http://ci.ignite.apache.org/viewQueued.html?itemId=734208
>
>
> On 19.07.2017 17:55, Dmitry Pavlov wrote:
>> Hi Evgeniy,
>>
>> Thank you. Ignite Basic is one suite from approximately 80 suites that
>> covers Ignite by automated tests. Which is why I suggested to use RunAll
>> chain in ignite 2.0 group. Yes, several tests may fail, especially if
>> it is
>> flaky tests or failure is related to the specific JIRA issue.
>>
>> About change itself: This change seems to be very impact. There is
>> possiblity that many of existing plugins relies on existing order of
>> initialization. This change may break plugin initialization in
>> unexpected
>> manner.
>>
>> Could we
>> - fix javadoc according to existing order in code
>> - find out new solution?
>>
>> Sincerely,
>> Dmitriy Pavlov
>>
>>
>> ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev
>> <[hidden email]>:
>>
>>> http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic 
>>>
>>> - this one - there seem to be no new failed platform tests, other
>>> failed
>>> tests seem to fail in several other reviews too and are unrelated to my
>>> changes.
>>>
>>>
>>> On 19.07.2017 17:35, Dmitry Pavlov wrote:
>>>> Hi Evgeniy,
>>>>
>>>> I was not able to find Teamcity run for this change.
>>>> Could you please run http://ci.ignite.apache.org test for example on
>>> branch
>>>> pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
>>>> Or could you please share link to previous run on this changes?
>>>>
>>>> Sincerely,
>>>> Dmitriy Pavlov
>>>>
>>>> ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
>>>>
>>>>> Igniters,
>>>>>
>>>>> Could somebody review the fix today?
>>>>>
>>>>> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
>>>>> [hidden email]> wrote:
>>>>>
>>>>>> Hello, Igniters.
>>>>>>
>>>>>> Could anyone review my request - https://issues.apache.org/jira
>>>>>> /browse/IGNITE-5123? - My previous pings seems to got lost.
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Yevgeniy
>>>>>>
>>>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Dmitriy Pavlov
Hi Evgeniy,

I've checked the results. All of failures are flaky tests, failed with
issue link or were failed somewhere else.

Sincerely,
Dmitriy Pavlov

чт, 20 июл. 2017 г. в 11:48, Evgeniy Ignatiev <[hidden email]>:

> Rerun tests in "Run All" Ignite 2.0 group after merge with the latest
> master -
>
> http://ci.ignite.apache.org/viewLog.html?buildId=734705&tab=queuedBuildOverviewTab
> - Do you see any issues with them? (Unfortunately I have too little
> experience with Ignite tests to judge.)
>
>
> On 7/19/2017 6:25 PM, Evgeniy Ignatiev wrote:
> > Scheduled "Run All" in Ignite 2.0 Tests for pull/2285/head -
> > http://ci.ignite.apache.org/viewQueued.html?itemId=734208
> >
> >
> > On 19.07.2017 17:55, Dmitry Pavlov wrote:
> >> Hi Evgeniy,
> >>
> >> Thank you. Ignite Basic is one suite from approximately 80 suites that
> >> covers Ignite by automated tests. Which is why I suggested to use RunAll
> >> chain in ignite 2.0 group. Yes, several tests may fail, especially if
> >> it is
> >> flaky tests or failure is related to the specific JIRA issue.
> >>
> >> About change itself: This change seems to be very impact. There is
> >> possiblity that many of existing plugins relies on existing order of
> >> initialization. This change may break plugin initialization in
> >> unexpected
> >> manner.
> >>
> >> Could we
> >> - fix javadoc according to existing order in code
> >> - find out new solution?
> >>
> >> Sincerely,
> >> Dmitriy Pavlov
> >>
> >>
> >> ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev
> >> <[hidden email]>:
> >>
> >>>
> http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
> >>>
> >>> - this one - there seem to be no new failed platform tests, other
> >>> failed
> >>> tests seem to fail in several other reviews too and are unrelated to my
> >>> changes.
> >>>
> >>>
> >>> On 19.07.2017 17:35, Dmitry Pavlov wrote:
> >>>> Hi Evgeniy,
> >>>>
> >>>> I was not able to find Teamcity run for this change.
> >>>> Could you please run http://ci.ignite.apache.org test for example on
> >>> branch
> >>>> pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
> >>>> Or could you please share link to previous run on this changes?
> >>>>
> >>>> Sincerely,
> >>>> Dmitriy Pavlov
> >>>>
> >>>> ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
> >>>>
> >>>>> Igniters,
> >>>>>
> >>>>> Could somebody review the fix today?
> >>>>>
> >>>>> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
> >>>>> [hidden email]> wrote:
> >>>>>
> >>>>>> Hello, Igniters.
> >>>>>>
> >>>>>> Could anyone review my request - https://issues.apache.org/jira
> >>>>>> /browse/IGNITE-5123? - My previous pings seems to got lost.
> >>>>>>
> >>>>>> Best regards,
> >>>>>>
> >>>>>> Yevgeniy
> >>>>>>
> >>>>>>
> >>>
> >
>
>
Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Dmitriy Pavlov
In reply to this post by npordash
Hi Nick,

Thank you for your comment. Was onIgniteStart called after onKernalStart in
1.9? Or caches were available, but other of initialization was the same?

Sincerely.
Dmitriy Pavlov

ср, 19 июл. 2017 г. в 17:06, Nick Pordash <[hidden email]>:

> Hi Dmitriy,
>
> The ticket was a regression from 1.9 to 2.0. I don't think anyone would be
> expecting the behavior in 2.0 as it doesn't align with the javadoc and has
> only been broken since the 2.0 release.
>
> -Nick
>
> On Wed, Jul 19, 2017, 6:55 AM Dmitry Pavlov <[hidden email]> wrote:
>
> > Hi Evgeniy,
> >
> > Thank you. Ignite Basic is one suite from approximately 80 suites that
> > covers Ignite by automated tests. Which is why I suggested to use RunAll
> > chain in ignite 2.0 group. Yes, several tests may fail, especially if it
> is
> > flaky tests or failure is related to the specific JIRA issue.
> >
> > About change itself: This change seems to be very impact. There is
> > possiblity that many of existing plugins relies on existing order of
> > initialization. This change may break plugin initialization in unexpected
> > manner.
> >
> > Could we
> > - fix javadoc according to existing order in code
> > - find out new solution?
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> >
> > ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev <
> [hidden email]
> > >:
> >
> > >
> > >
> >
> http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
> > > - this one - there seem to be no new failed platform tests, other
> failed
> > > tests seem to fail in several other reviews too and are unrelated to my
> > > changes.
> > >
> > >
> > > On 19.07.2017 17:35, Dmitry Pavlov wrote:
> > > > Hi Evgeniy,
> > > >
> > > > I was not able to find Teamcity run for this change.
> > > > Could you please run http://ci.ignite.apache.org test for example on
> > > branch
> > > > pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
> > > > Or could you please share link to previous run on this changes?
> > > >
> > > > Sincerely,
> > > > Dmitriy Pavlov
> > > >
> > > > ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
> > > >
> > > >> Igniters,
> > > >>
> > > >> Could somebody review the fix today?
> > > >>
> > > >> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
> > > >> [hidden email]> wrote:
> > > >>
> > > >>> Hello, Igniters.
> > > >>>
> > > >>> Could anyone review my request - https://issues.apache.org/jira
> > > >>> /browse/IGNITE-5123? - My previous pings seems to got lost.
> > > >>>
> > > >>> Best regards,
> > > >>>
> > > >>> Yevgeniy
> > > >>>
> > > >>>
> > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Evgeniy Ignatiev
onIgniteStart was called in Ignite 1.X in
GridPluginComponent#onKernalStart as one of the calls to the component
callbacks, probably the order in which components were called, ensured
that contract of PluginProvider#onIgniteStart was not violated. But in
2.0 the GridPluginComponent instances are explicitly skipped from this
cycle (lines 1019-1020 in Ignite 2.0 release source) and PluginProviders
are notified before the internal component callbacks.

As far as I can see the change, that moved PluginProvider#onIgniteStart
notification before component callbacks, was introduced by this commit -
https://github.com/apache/ignite/commit/6b7bf97158c097b80bcf5c2150e67a5210269e6d 
- but I have no clue what was the reason.


On 7/20/2017 7:51 PM, Dmitry Pavlov wrote:

> Hi Nick,
>
> Thank you for your comment. Was onIgniteStart called after onKernalStart in
> 1.9? Or caches were available, but other of initialization was the same?
>
> Sincerely.
> Dmitriy Pavlov
>
> ср, 19 июл. 2017 г. в 17:06, Nick Pordash <[hidden email]>:
>
>> Hi Dmitriy,
>>
>> The ticket was a regression from 1.9 to 2.0. I don't think anyone would be
>> expecting the behavior in 2.0 as it doesn't align with the javadoc and has
>> only been broken since the 2.0 release.
>>
>> -Nick
>>
>> On Wed, Jul 19, 2017, 6:55 AM Dmitry Pavlov <[hidden email]> wrote:
>>
>>> Hi Evgeniy,
>>>
>>> Thank you. Ignite Basic is one suite from approximately 80 suites that
>>> covers Ignite by automated tests. Which is why I suggested to use RunAll
>>> chain in ignite 2.0 group. Yes, several tests may fail, especially if it
>> is
>>> flaky tests or failure is related to the specific JIRA issue.
>>>
>>> About change itself: This change seems to be very impact. There is
>>> possiblity that many of existing plugins relies on existing order of
>>> initialization. This change may break plugin initialization in unexpected
>>> manner.
>>>
>>> Could we
>>> - fix javadoc according to existing order in code
>>> - find out new solution?
>>>
>>> Sincerely,
>>> Dmitriy Pavlov
>>>
>>>
>>> ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev <
>> [hidden email]
>>>> :
>>>>
>> http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
>>>> - this one - there seem to be no new failed platform tests, other
>> failed
>>>> tests seem to fail in several other reviews too and are unrelated to my
>>>> changes.
>>>>
>>>>
>>>> On 19.07.2017 17:35, Dmitry Pavlov wrote:
>>>>> Hi Evgeniy,
>>>>>
>>>>> I was not able to find Teamcity run for this change.
>>>>> Could you please run http://ci.ignite.apache.org test for example on
>>>> branch
>>>>> pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
>>>>> Or could you please share link to previous run on this changes?
>>>>>
>>>>> Sincerely,
>>>>> Dmitriy Pavlov
>>>>>
>>>>> ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
>>>>>
>>>>>> Igniters,
>>>>>>
>>>>>> Could somebody review the fix today?
>>>>>>
>>>>>> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
>>>>>> [hidden email]> wrote:
>>>>>>
>>>>>>> Hello, Igniters.
>>>>>>>
>>>>>>> Could anyone review my request - https://issues.apache.org/jira
>>>>>>> /browse/IGNITE-5123? - My previous pings seems to got lost.
>>>>>>>
>>>>>>> Best regards,
>>>>>>>
>>>>>>> Yevgeniy
>>>>>>>
>>>>>>>
>>>>

Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Dmitriy Pavlov
Hi Evgeniy,

Thank you for such a careful research of the issue.

If don’t mind, I would like to do additional tests with this PR changes.

I will come back with result in couple of days

Sincerely,
Dmitriy Pavlov

чт, 20 июл. 2017 г. в 19:18, Evgeniy Ignatiev <[hidden email]>:

> onIgniteStart was called in Ignite 1.X in
> GridPluginComponent#onKernalStart as one of the calls to the component
> callbacks, probably the order in which components were called, ensured
> that contract of PluginProvider#onIgniteStart was not violated. But in
> 2.0 the GridPluginComponent instances are explicitly skipped from this
> cycle (lines 1019-1020 in Ignite 2.0 release source) and PluginProviders
> are notified before the internal component callbacks.
>
> As far as I can see the change, that moved PluginProvider#onIgniteStart
> notification before component callbacks, was introduced by this commit -
>
> https://github.com/apache/ignite/commit/6b7bf97158c097b80bcf5c2150e67a5210269e6d
> - but I have no clue what was the reason.
>
>
> On 7/20/2017 7:51 PM, Dmitry Pavlov wrote:
> > Hi Nick,
> >
> > Thank you for your comment. Was onIgniteStart called after onKernalStart
> in
> > 1.9? Or caches were available, but other of initialization was the same?
> >
> > Sincerely.
> > Dmitriy Pavlov
> >
> > ср, 19 июл. 2017 г. в 17:06, Nick Pordash <[hidden email]>:
> >
> >> Hi Dmitriy,
> >>
> >> The ticket was a regression from 1.9 to 2.0. I don't think anyone would
> be
> >> expecting the behavior in 2.0 as it doesn't align with the javadoc and
> has
> >> only been broken since the 2.0 release.
> >>
> >> -Nick
> >>
> >> On Wed, Jul 19, 2017, 6:55 AM Dmitry Pavlov <[hidden email]>
> wrote:
> >>
> >>> Hi Evgeniy,
> >>>
> >>> Thank you. Ignite Basic is one suite from approximately 80 suites that
> >>> covers Ignite by automated tests. Which is why I suggested to use
> RunAll
> >>> chain in ignite 2.0 group. Yes, several tests may fail, especially if
> it
> >> is
> >>> flaky tests or failure is related to the specific JIRA issue.
> >>>
> >>> About change itself: This change seems to be very impact. There is
> >>> possiblity that many of existing plugins relies on existing order of
> >>> initialization. This change may break plugin initialization in
> unexpected
> >>> manner.
> >>>
> >>> Could we
> >>> - fix javadoc according to existing order in code
> >>> - find out new solution?
> >>>
> >>> Sincerely,
> >>> Dmitriy Pavlov
> >>>
> >>>
> >>> ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev <
> >> [hidden email]
> >>>> :
> >>>>
> >>
> http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
> >>>> - this one - there seem to be no new failed platform tests, other
> >> failed
> >>>> tests seem to fail in several other reviews too and are unrelated to
> my
> >>>> changes.
> >>>>
> >>>>
> >>>> On 19.07.2017 17:35, Dmitry Pavlov wrote:
> >>>>> Hi Evgeniy,
> >>>>>
> >>>>> I was not able to find Teamcity run for this change.
> >>>>> Could you please run http://ci.ignite.apache.org test for example on
> >>>> branch
> >>>>> pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
> >>>>> Or could you please share link to previous run on this changes?
> >>>>>
> >>>>> Sincerely,
> >>>>> Dmitriy Pavlov
> >>>>>
> >>>>> ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
> >>>>>
> >>>>>> Igniters,
> >>>>>>
> >>>>>> Could somebody review the fix today?
> >>>>>>
> >>>>>> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
> >>>>>> [hidden email]> wrote:
> >>>>>>
> >>>>>>> Hello, Igniters.
> >>>>>>>
> >>>>>>> Could anyone review my request - https://issues.apache.org/jira
> >>>>>>> /browse/IGNITE-5123? - My previous pings seems to got lost.
> >>>>>>>
> >>>>>>> Best regards,
> >>>>>>>
> >>>>>>> Yevgeniy
> >>>>>>>
> >>>>>>>
> >>>>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Dmitriy Pavlov
Hi Evgeniy,

From my point of view there are no problems with this fix. My testing
didn't show any issues with fix.

Igniters,

Are there any additional comments on this issue? Can we proceed?

Sincerely,
Dmitriy Pavlov

чт, 20 июл. 2017 г. в 20:04, Dmitry Pavlov <[hidden email]>:

> Hi Evgeniy,
>
> Thank you for such a careful research of the issue.
>
> If don’t mind, I would like to do additional tests with this PR changes.
>
> I will come back with result in couple of days
>
> Sincerely,
> Dmitriy Pavlov
>
> чт, 20 июл. 2017 г. в 19:18, Evgeniy Ignatiev <[hidden email]
> >:
>
>> onIgniteStart was called in Ignite 1.X in
>> GridPluginComponent#onKernalStart as one of the calls to the component
>> callbacks, probably the order in which components were called, ensured
>> that contract of PluginProvider#onIgniteStart was not violated. But in
>> 2.0 the GridPluginComponent instances are explicitly skipped from this
>> cycle (lines 1019-1020 in Ignite 2.0 release source) and PluginProviders
>> are notified before the internal component callbacks.
>>
>> As far as I can see the change, that moved PluginProvider#onIgniteStart
>> notification before component callbacks, was introduced by this commit -
>>
>> https://github.com/apache/ignite/commit/6b7bf97158c097b80bcf5c2150e67a5210269e6d
>> - but I have no clue what was the reason.
>>
>>
>> On 7/20/2017 7:51 PM, Dmitry Pavlov wrote:
>> > Hi Nick,
>> >
>> > Thank you for your comment. Was onIgniteStart called after
>> onKernalStart in
>> > 1.9? Or caches were available, but other of initialization was the same?
>> >
>> > Sincerely.
>> > Dmitriy Pavlov
>> >
>> > ср, 19 июл. 2017 г. в 17:06, Nick Pordash <[hidden email]>:
>> >
>> >> Hi Dmitriy,
>> >>
>> >> The ticket was a regression from 1.9 to 2.0. I don't think anyone
>> would be
>> >> expecting the behavior in 2.0 as it doesn't align with the javadoc and
>> has
>> >> only been broken since the 2.0 release.
>> >>
>> >> -Nick
>> >>
>> >> On Wed, Jul 19, 2017, 6:55 AM Dmitry Pavlov <[hidden email]>
>> wrote:
>> >>
>> >>> Hi Evgeniy,
>> >>>
>> >>> Thank you. Ignite Basic is one suite from approximately 80 suites that
>> >>> covers Ignite by automated tests. Which is why I suggested to use
>> RunAll
>> >>> chain in ignite 2.0 group. Yes, several tests may fail, especially if
>> it
>> >> is
>> >>> flaky tests or failure is related to the specific JIRA issue.
>> >>>
>> >>> About change itself: This change seems to be very impact. There is
>> >>> possiblity that many of existing plugins relies on existing order of
>> >>> initialization. This change may break plugin initialization in
>> unexpected
>> >>> manner.
>> >>>
>> >>> Could we
>> >>> - fix javadoc according to existing order in code
>> >>> - find out new solution?
>> >>>
>> >>> Sincerely,
>> >>> Dmitriy Pavlov
>> >>>
>> >>>
>> >>> ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev <
>> >> [hidden email]
>> >>>> :
>> >>>>
>> >>
>> http://ci.ignite.apache.org/viewLog.html?buildId=720722&tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
>> >>>> - this one - there seem to be no new failed platform tests, other
>> >> failed
>> >>>> tests seem to fail in several other reviews too and are unrelated to
>> my
>> >>>> changes.
>> >>>>
>> >>>>
>> >>>> On 19.07.2017 17:35, Dmitry Pavlov wrote:
>> >>>>> Hi Evgeniy,
>> >>>>>
>> >>>>> I was not able to find Teamcity run for this change.
>> >>>>> Could you please run http://ci.ignite.apache.org test for example
>> on
>> >>>> branch
>> >>>>> pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
>> >>>>> Or could you please share link to previous run on this changes?
>> >>>>>
>> >>>>> Sincerely,
>> >>>>> Dmitriy Pavlov
>> >>>>>
>> >>>>> ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
>> >>>>>
>> >>>>>> Igniters,
>> >>>>>>
>> >>>>>> Could somebody review the fix today?
>> >>>>>>
>> >>>>>> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
>> >>>>>> [hidden email]> wrote:
>> >>>>>>
>> >>>>>>> Hello, Igniters.
>> >>>>>>>
>> >>>>>>> Could anyone review my request - https://issues.apache.org/jira
>> >>>>>>> /browse/IGNITE-5123? - My previous pings seems to got lost.
>> >>>>>>>
>> >>>>>>> Best regards,
>> >>>>>>>
>> >>>>>>> Yevgeniy
>> >>>>>>>
>> >>>>>>>
>> >>>>
>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Semyon Boikov
Evgeniy and Dmitry, thanks for the fix! Merged into master.

Thanks!

On Sun, Jul 23, 2017 at 9:21 PM, Dmitry Pavlov <[hidden email]>
wrote:

> Hi Evgeniy,
>
> From my point of view there are no problems with this fix. My testing
> didn't show any issues with fix.
>
> Igniters,
>
> Are there any additional comments on this issue? Can we proceed?
>
> Sincerely,
> Dmitriy Pavlov
>
> чт, 20 июл. 2017 г. в 20:04, Dmitry Pavlov <[hidden email]>:
>
> > Hi Evgeniy,
> >
> > Thank you for such a careful research of the issue.
> >
> > If don’t mind, I would like to do additional tests with this PR changes.
> >
> > I will come back with result in couple of days
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > чт, 20 июл. 2017 г. в 19:18, Evgeniy Ignatiev <
> [hidden email]
> > >:
> >
> >> onIgniteStart was called in Ignite 1.X in
> >> GridPluginComponent#onKernalStart as one of the calls to the component
> >> callbacks, probably the order in which components were called, ensured
> >> that contract of PluginProvider#onIgniteStart was not violated. But in
> >> 2.0 the GridPluginComponent instances are explicitly skipped from this
> >> cycle (lines 1019-1020 in Ignite 2.0 release source) and PluginProviders
> >> are notified before the internal component callbacks.
> >>
> >> As far as I can see the change, that moved PluginProvider#onIgniteStart
> >> notification before component callbacks, was introduced by this commit -
> >>
> >> https://github.com/apache/ignite/commit/6b7bf97158c097b80bcf5c2150e67a
> 5210269e6d
> >> - but I have no clue what was the reason.
> >>
> >>
> >> On 7/20/2017 7:51 PM, Dmitry Pavlov wrote:
> >> > Hi Nick,
> >> >
> >> > Thank you for your comment. Was onIgniteStart called after
> >> onKernalStart in
> >> > 1.9? Or caches were available, but other of initialization was the
> same?
> >> >
> >> > Sincerely.
> >> > Dmitriy Pavlov
> >> >
> >> > ср, 19 июл. 2017 г. в 17:06, Nick Pordash <[hidden email]>:
> >> >
> >> >> Hi Dmitriy,
> >> >>
> >> >> The ticket was a regression from 1.9 to 2.0. I don't think anyone
> >> would be
> >> >> expecting the behavior in 2.0 as it doesn't align with the javadoc
> and
> >> has
> >> >> only been broken since the 2.0 release.
> >> >>
> >> >> -Nick
> >> >>
> >> >> On Wed, Jul 19, 2017, 6:55 AM Dmitry Pavlov <[hidden email]>
> >> wrote:
> >> >>
> >> >>> Hi Evgeniy,
> >> >>>
> >> >>> Thank you. Ignite Basic is one suite from approximately 80 suites
> that
> >> >>> covers Ignite by automated tests. Which is why I suggested to use
> >> RunAll
> >> >>> chain in ignite 2.0 group. Yes, several tests may fail, especially
> if
> >> it
> >> >> is
> >> >>> flaky tests or failure is related to the specific JIRA issue.
> >> >>>
> >> >>> About change itself: This change seems to be very impact. There is
> >> >>> possiblity that many of existing plugins relies on existing order of
> >> >>> initialization. This change may break plugin initialization in
> >> unexpected
> >> >>> manner.
> >> >>>
> >> >>> Could we
> >> >>> - fix javadoc according to existing order in code
> >> >>> - find out new solution?
> >> >>>
> >> >>> Sincerely,
> >> >>> Dmitriy Pavlov
> >> >>>
> >> >>>
> >> >>> ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev <
> >> >> [hidden email]
> >> >>>> :
> >> >>>>
> >> >>
> >> http://ci.ignite.apache.org/viewLog.html?buildId=720722&
> tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
> >> >>>> - this one - there seem to be no new failed platform tests, other
> >> >> failed
> >> >>>> tests seem to fail in several other reviews too and are unrelated
> to
> >> my
> >> >>>> changes.
> >> >>>>
> >> >>>>
> >> >>>> On 19.07.2017 17:35, Dmitry Pavlov wrote:
> >> >>>>> Hi Evgeniy,
> >> >>>>>
> >> >>>>> I was not able to find Teamcity run for this change.
> >> >>>>> Could you please run http://ci.ignite.apache.org test for example
> >> on
> >> >>>> branch
> >> >>>>> pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
> >> >>>>> Or could you please share link to previous run on this changes?
> >> >>>>>
> >> >>>>> Sincerely,
> >> >>>>> Dmitriy Pavlov
> >> >>>>>
> >> >>>>> ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
> >> >>>>>
> >> >>>>>> Igniters,
> >> >>>>>>
> >> >>>>>> Could somebody review the fix today?
> >> >>>>>>
> >> >>>>>> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
> >> >>>>>> [hidden email]> wrote:
> >> >>>>>>
> >> >>>>>>> Hello, Igniters.
> >> >>>>>>>
> >> >>>>>>> Could anyone review my request - https://issues.apache.org/jira
> >> >>>>>>> /browse/IGNITE-5123? - My previous pings seems to got lost.
> >> >>>>>>>
> >> >>>>>>> Best regards,
> >> >>>>>>>
> >> >>>>>>> Yevgeniy
> >> >>>>>>>
> >> >>>>>>>
> >> >>>>
> >>
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: IGNITE-5123 Review

Evgeniy Ignatiev
Thank you! Many thanks to Dmitry for his attitude and dedication!


On 24.07.2017 16:47, Semyon Boikov wrote:

> Evgeniy and Dmitry, thanks for the fix! Merged into master.
>
> Thanks!
>
> On Sun, Jul 23, 2017 at 9:21 PM, Dmitry Pavlov <[hidden email]>
> wrote:
>
>> Hi Evgeniy,
>>
>>  From my point of view there are no problems with this fix. My testing
>> didn't show any issues with fix.
>>
>> Igniters,
>>
>> Are there any additional comments on this issue? Can we proceed?
>>
>> Sincerely,
>> Dmitriy Pavlov
>>
>> чт, 20 июл. 2017 г. в 20:04, Dmitry Pavlov <[hidden email]>:
>>
>>> Hi Evgeniy,
>>>
>>> Thank you for such a careful research of the issue.
>>>
>>> If don’t mind, I would like to do additional tests with this PR changes.
>>>
>>> I will come back with result in couple of days
>>>
>>> Sincerely,
>>> Dmitriy Pavlov
>>>
>>> чт, 20 июл. 2017 г. в 19:18, Evgeniy Ignatiev <
>> [hidden email]
>>>> :
>>>> onIgniteStart was called in Ignite 1.X in
>>>> GridPluginComponent#onKernalStart as one of the calls to the component
>>>> callbacks, probably the order in which components were called, ensured
>>>> that contract of PluginProvider#onIgniteStart was not violated. But in
>>>> 2.0 the GridPluginComponent instances are explicitly skipped from this
>>>> cycle (lines 1019-1020 in Ignite 2.0 release source) and PluginProviders
>>>> are notified before the internal component callbacks.
>>>>
>>>> As far as I can see the change, that moved PluginProvider#onIgniteStart
>>>> notification before component callbacks, was introduced by this commit -
>>>>
>>>> https://github.com/apache/ignite/commit/6b7bf97158c097b80bcf5c2150e67a
>> 5210269e6d
>>>> - but I have no clue what was the reason.
>>>>
>>>>
>>>> On 7/20/2017 7:51 PM, Dmitry Pavlov wrote:
>>>>> Hi Nick,
>>>>>
>>>>> Thank you for your comment. Was onIgniteStart called after
>>>> onKernalStart in
>>>>> 1.9? Or caches were available, but other of initialization was the
>> same?
>>>>> Sincerely.
>>>>> Dmitriy Pavlov
>>>>>
>>>>> ср, 19 июл. 2017 г. в 17:06, Nick Pordash <[hidden email]>:
>>>>>
>>>>>> Hi Dmitriy,
>>>>>>
>>>>>> The ticket was a regression from 1.9 to 2.0. I don't think anyone
>>>> would be
>>>>>> expecting the behavior in 2.0 as it doesn't align with the javadoc
>> and
>>>> has
>>>>>> only been broken since the 2.0 release.
>>>>>>
>>>>>> -Nick
>>>>>>
>>>>>> On Wed, Jul 19, 2017, 6:55 AM Dmitry Pavlov <[hidden email]>
>>>> wrote:
>>>>>>> Hi Evgeniy,
>>>>>>>
>>>>>>> Thank you. Ignite Basic is one suite from approximately 80 suites
>> that
>>>>>>> covers Ignite by automated tests. Which is why I suggested to use
>>>> RunAll
>>>>>>> chain in ignite 2.0 group. Yes, several tests may fail, especially
>> if
>>>> it
>>>>>> is
>>>>>>> flaky tests or failure is related to the specific JIRA issue.
>>>>>>>
>>>>>>> About change itself: This change seems to be very impact. There is
>>>>>>> possiblity that many of existing plugins relies on existing order of
>>>>>>> initialization. This change may break plugin initialization in
>>>> unexpected
>>>>>>> manner.
>>>>>>>
>>>>>>> Could we
>>>>>>> - fix javadoc according to existing order in code
>>>>>>> - find out new solution?
>>>>>>>
>>>>>>> Sincerely,
>>>>>>> Dmitriy Pavlov
>>>>>>>
>>>>>>>
>>>>>>> ср, 19 июл. 2017 г. в 16:40, Evgeniy Ignatiev <
>>>>>> [hidden email]
>>>>>>>> :
>>>>>>>>
>>>> http://ci.ignite.apache.org/viewLog.html?buildId=720722&
>> tab=buildResultsDiv&buildTypeId=IgniteTests_IgniteBasic
>>>>>>>> - this one - there seem to be no new failed platform tests, other
>>>>>> failed
>>>>>>>> tests seem to fail in several other reviews too and are unrelated
>> to
>>>> my
>>>>>>>> changes.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 19.07.2017 17:35, Dmitry Pavlov wrote:
>>>>>>>>> Hi Evgeniy,
>>>>>>>>>
>>>>>>>>> I was not able to find Teamcity run for this change.
>>>>>>>>> Could you please run http://ci.ignite.apache.org test for example
>>>> on
>>>>>>>> branch
>>>>>>>>> pull/2285/head using 'Ignite 2.0 Tests' target 'Run All'.
>>>>>>>>> Or could you please share link to previous run on this changes?
>>>>>>>>>
>>>>>>>>> Sincerely,
>>>>>>>>> Dmitriy Pavlov
>>>>>>>>>
>>>>>>>>> ср, 19 июл. 2017 г. в 15:26, Anton Vinogradov <[hidden email]>:
>>>>>>>>>
>>>>>>>>>> Igniters,
>>>>>>>>>>
>>>>>>>>>> Could somebody review the fix today?
>>>>>>>>>>
>>>>>>>>>> On Wed, Jul 19, 2017 at 1:30 PM, Evgeniy Ignatiev <
>>>>>>>>>> [hidden email]> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hello, Igniters.
>>>>>>>>>>>
>>>>>>>>>>> Could anyone review my request - https://issues.apache.org/jira
>>>>>>>>>>> /browse/IGNITE-5123? - My previous pings seems to got lost.
>>>>>>>>>>>
>>>>>>>>>>> Best regards,
>>>>>>>>>>>
>>>>>>>>>>> Yevgeniy
>>>>>>>>>>>
>>>>>>>>>>>
>>>>