Re: Semaphore blocking on tryAcquire() while holding a cache-lock

[Alexey Goncharuk]

Guys,

I fixed code style a bit and pushed my changes to the branch.

Couple of questions:
 - I see that some of the Errors caught do not get re-thrown (e.g. if
interruptAll flag is set). I believe we should at least re-throw OOME in
any case.
 - readResolve method is missing for CacheLockImpl. The current
readExternal/writeExternal code uses static stash field. I looked around in
the code and found that IgniteKernal uses localIgnite, while
GridCacheAdapter uses stash. Which way is the correct one?
​

[yzhdanov]

Very good points, Alexey. I will look at this tomorrow and finalize the
changes.

--Yakov

2016-04-12 23:41 GMT+03:00 Alexey Goncharuk <[hidden email]>:

> Guys,
>
> I fixed code style a bit and pushed my changes to the branch.
>
> Couple of questions:
>  - I see that some of the Errors caught do not get re-thrown (e.g. if
> interruptAll flag is set). I believe we should at least re-throw OOME in
> any case.
>  - readResolve method is missing for CacheLockImpl. The current
> readExternal/writeExternal code uses static stash field. I looked around in
> the code and found that IgniteKernal uses localIgnite, while
> GridCacheAdapter uses stash. Which way is the correct one?
> ​
>

[yzhdanov]

Vlad, please see my changes in ignite-642 and comment in the ticket.

Alex, can you please take a look at my latest commit as well and provide
comments?

--Yakov

2016-04-12 23:47 GMT+03:00 Yakov Zhdanov <[hidden email]>:

> Very good points, Alexey. I will look at this tomorrow and finalize the
> changes.
>
> --Yakov
>
> 2016-04-12 23:41 GMT+03:00 Alexey Goncharuk <[hidden email]>:
>
>> Guys,
>>
>> I fixed code style a bit and pushed my changes to the branch.
>>
>> Couple of questions:
>>  - I see that some of the Errors caught do not get re-thrown (e.g. if
>> interruptAll flag is set). I believe we should at least re-throw OOME in
>> any case.
>>  - readResolve method is missing for CacheLockImpl. The current
>> readExternal/writeExternal code uses static stash field. I looked around
>> in
>> the code and found that IgniteKernal uses localIgnite, while
>> GridCacheAdapter uses stash. Which way is the correct one?
>> ​
>>
>
>

[Vladisav Jelisavcic]

Sure, I'll look into it later today, or tomorrow at the latest

On Thu, Apr 14, 2016 at 5:53 PM, Yakov Zhdanov <[hidden email]> wrote:

> Vlad, please see my changes in ignite-642 and comment in the ticket.
>
> Alex, can you please take a look at my latest commit as well and provide
> comments?
>
> --Yakov
>
> 2016-04-12 23:47 GMT+03:00 Yakov Zhdanov <[hidden email]>:
>
> > Very good points, Alexey. I will look at this tomorrow and finalize the
> > changes.
> >
> > --Yakov
> >
> > 2016-04-12 23:41 GMT+03:00 Alexey Goncharuk <[hidden email]
> >:
> >
> >> Guys,
> >>
> >> I fixed code style a bit and pushed my changes to the branch.
> >>
> >> Couple of questions:
> >>  - I see that some of the Errors caught do not get re-thrown (e.g. if
> >> interruptAll flag is set). I believe we should at least re-throw OOME in
> >> any case.
> >>  - readResolve method is missing for CacheLockImpl. The current
> >> readExternal/writeExternal code uses static stash field. I looked around
> >> in
> >> the code and found that IgniteKernal uses localIgnite, while
> >> GridCacheAdapter uses stash. Which way is the correct one?
> >> ​
> >>
> >
> >
>

[Vladisav Jelisavcic]

Yakov,

I reviewed the changes in ignite-642 and it looks good to me, but I have
one question.
Can you please look at my comment in ignite-642 ticket?

Thanks!
Vladisav

On Thu, Apr 14, 2016 at 7:51 PM, Vladisav Jelisavcic <[hidden email]>
wrote:

> Sure, I'll look into it later today, or tomorrow at the latest
>
> On Thu, Apr 14, 2016 at 5:53 PM, Yakov Zhdanov <[hidden email]>
> wrote:
>
>> Vlad, please see my changes in ignite-642 and comment in the ticket.
>>
>> Alex, can you please take a look at my latest commit as well and provide
>> comments?
>>
>> --Yakov
>>
>> 2016-04-12 23:47 GMT+03:00 Yakov Zhdanov <[hidden email]>:
>>
>> > Very good points, Alexey. I will look at this tomorrow and finalize the
>> > changes.
>> >
>> > --Yakov
>> >
>> > 2016-04-12 23:41 GMT+03:00 Alexey Goncharuk <[hidden email]
>> >:
>> >
>> >> Guys,
>> >>
>> >> I fixed code style a bit and pushed my changes to the branch.
>> >>
>> >> Couple of questions:
>> >>  - I see that some of the Errors caught do not get re-thrown (e.g. if
>> >> interruptAll flag is set). I believe we should at least re-throw OOME
>> in
>> >> any case.
>> >>  - readResolve method is missing for CacheLockImpl. The current
>> >> readExternal/writeExternal code uses static stash field. I looked
>> around
>> >> in
>> >> the code and found that IgniteKernal uses localIgnite, while
>> >> GridCacheAdapter uses stash. Which way is the correct one?
>> >> ​
>> >>
>> >
>> >
>>
>
>

[Vladisav Jelisavcic]

Yakov,

I've finished the initialization tests for ignite-642 (and moved
serialization test from GridCacheLockAbstractTest to
IgniteLockAbstractSelfTest).
Please check the commit and let me know if you spot anything else.
Thanks!

On Fri, Apr 15, 2016 at 10:11 AM, Vladisav Jelisavcic <[hidden email]>
wrote:

> Yakov,
>
> I reviewed the changes in ignite-642 and it looks good to me, but I have
> one question.
> Can you please look at my comment in ignite-642 ticket?
>
> Thanks!
> Vladisav
>
> On Thu, Apr 14, 2016 at 7:51 PM, Vladisav Jelisavcic <[hidden email]>
> wrote:
>
>> Sure, I'll look into it later today, or tomorrow at the latest
>>
>> On Thu, Apr 14, 2016 at 5:53 PM, Yakov Zhdanov <[hidden email]>
>> wrote:
>>
>>> Vlad, please see my changes in ignite-642 and comment in the ticket.
>>>
>>> Alex, can you please take a look at my latest commit as well and provide
>>> comments?
>>>
>>> --Yakov
>>>
>>> 2016-04-12 23:47 GMT+03:00 Yakov Zhdanov <[hidden email]>:
>>>
>>> > Very good points, Alexey. I will look at this tomorrow and finalize the
>>> > changes.
>>> >
>>> > --Yakov
>>> >
>>> > 2016-04-12 23:41 GMT+03:00 Alexey Goncharuk <
>>> [hidden email]>:
>>> >
>>> >> Guys,
>>> >>
>>> >> I fixed code style a bit and pushed my changes to the branch.
>>> >>
>>> >> Couple of questions:
>>> >>  - I see that some of the Errors caught do not get re-thrown (e.g. if
>>> >> interruptAll flag is set). I believe we should at least re-throw OOME
>>> in
>>> >> any case.
>>> >>  - readResolve method is missing for CacheLockImpl. The current
>>> >> readExternal/writeExternal code uses static stash field. I looked
>>> around
>>> >> in
>>> >> the code and found that IgniteKernal uses localIgnite, while
>>> >> GridCacheAdapter uses stash. Which way is the correct one?
>>> >> ​
>>> >>
>>> >
>>> >
>>>
>>
>>
>

[Vladisav Jelisavcic]

Yakov,

did you had time to do another review round of ignite-642?

Thanks!

On Fri, Apr 15, 2016 at 3:53 PM, Vladisav Jelisavcic <[hidden email]>
wrote:

> Yakov,
>
> I've finished the initialization tests for ignite-642 (and moved
> serialization test from GridCacheLockAbstractTest to
> IgniteLockAbstractSelfTest).
> Please check the commit and let me know if you spot anything else.
> Thanks!
>
> On Fri, Apr 15, 2016 at 10:11 AM, Vladisav Jelisavcic <[hidden email]
> > wrote:
>
>> Yakov,
>>
>> I reviewed the changes in ignite-642 and it looks good to me, but I have
>> one question.
>> Can you please look at my comment in ignite-642 ticket?
>>
>> Thanks!
>> Vladisav
>>
>> On Thu, Apr 14, 2016 at 7:51 PM, Vladisav Jelisavcic <[hidden email]
>> > wrote:
>>
>>> Sure, I'll look into it later today, or tomorrow at the latest
>>>
>>> On Thu, Apr 14, 2016 at 5:53 PM, Yakov Zhdanov <[hidden email]>
>>> wrote:
>>>
>>>> Vlad, please see my changes in ignite-642 and comment in the ticket.
>>>>
>>>> Alex, can you please take a look at my latest commit as well and provide
>>>> comments?
>>>>
>>>> --Yakov
>>>>
>>>> 2016-04-12 23:47 GMT+03:00 Yakov Zhdanov <[hidden email]>:
>>>>
>>>> > Very good points, Alexey. I will look at this tomorrow and finalize
>>>> the
>>>> > changes.
>>>> >
>>>> > --Yakov
>>>> >
>>>> > 2016-04-12 23:41 GMT+03:00 Alexey Goncharuk <
>>>> [hidden email]>:
>>>> >
>>>> >> Guys,
>>>> >>
>>>> >> I fixed code style a bit and pushed my changes to the branch.
>>>> >>
>>>> >> Couple of questions:
>>>> >>  - I see that some of the Errors caught do not get re-thrown (e.g. if
>>>> >> interruptAll flag is set). I believe we should at least re-throw
>>>> OOME in
>>>> >> any case.
>>>> >>  - readResolve method is missing for CacheLockImpl. The current
>>>> >> readExternal/writeExternal code uses static stash field. I looked
>>>> around
>>>> >> in
>>>> >> the code and found that IgniteKernal uses localIgnite, while
>>>> >> GridCacheAdapter uses stash. Which way is the correct one?
>>>> >> ​
>>>> >>
>>>> >
>>>> >
>>>>
>>>
>>>
>>
>

[yzhdanov]

Vlad, not yet, unfortunately. I will try to do today.

--Yakov

2016-04-27 11:40 GMT+03:00 Vladisav Jelisavcic <[hidden email]>:

> Yakov,
>
> did you had time to do another review round of ignite-642?
>
> Thanks!
>
> On Fri, Apr 15, 2016 at 3:53 PM, Vladisav Jelisavcic <[hidden email]>
> wrote:
>
> > Yakov,
> >
> > I've finished the initialization tests for ignite-642 (and moved
> > serialization test from GridCacheLockAbstractTest to
> > IgniteLockAbstractSelfTest).
> > Please check the commit and let me know if you spot anything else.
> > Thanks!
> >
> > On Fri, Apr 15, 2016 at 10:11 AM, Vladisav Jelisavcic <
> [hidden email]
> > > wrote:
> >
> >> Yakov,
> >>
> >> I reviewed the changes in ignite-642 and it looks good to me, but I have
> >> one question.
> >> Can you please look at my comment in ignite-642 ticket?
> >>
> >> Thanks!
> >> Vladisav
> >>
> >> On Thu, Apr 14, 2016 at 7:51 PM, Vladisav Jelisavcic <
> [hidden email]
> >> > wrote:
> >>
> >>> Sure, I'll look into it later today, or tomorrow at the latest
> >>>
> >>> On Thu, Apr 14, 2016 at 5:53 PM, Yakov Zhdanov <[hidden email]>
> >>> wrote:
> >>>
> >>>> Vlad, please see my changes in ignite-642 and comment in the ticket.
> >>>>
> >>>> Alex, can you please take a look at my latest commit as well and
> provide
> >>>> comments?
> >>>>
> >>>> --Yakov
> >>>>
> >>>> 2016-04-12 23:47 GMT+03:00 Yakov Zhdanov <[hidden email]>:
> >>>>
> >>>> > Very good points, Alexey. I will look at this tomorrow and finalize
> >>>> the
> >>>> > changes.
> >>>> >
> >>>> > --Yakov
> >>>> >
> >>>> > 2016-04-12 23:41 GMT+03:00 Alexey Goncharuk <
> >>>> [hidden email]>:
> >>>> >
> >>>> >> Guys,
> >>>> >>
> >>>> >> I fixed code style a bit and pushed my changes to the branch.
> >>>> >>
> >>>> >> Couple of questions:
> >>>> >>  - I see that some of the Errors caught do not get re-thrown (e.g.
> if
> >>>> >> interruptAll flag is set). I believe we should at least re-throw
> >>>> OOME in
> >>>> >> any case.
> >>>> >>  - readResolve method is missing for CacheLockImpl. The current
> >>>> >> readExternal/writeExternal code uses static stash field. I looked
> >>>> around
> >>>> >> in
> >>>> >> the code and found that IgniteKernal uses localIgnite, while
> >>>> >> GridCacheAdapter uses stash. Which way is the correct one?
> >>>> >> ​
> >>>> >>
> >>>> >
> >>>> >
> >>>>
> >>>
> >>>
> >>
> >
>

[yzhdanov]

Vlad and All!

I have reviewed and merged ignite-642 (
https://issues.apache.org/jira/browse/IGNITE-642 Implement
IgniteReentrantLock data structure) to master. I will send separate email
to user- and dev-list on this.



--Yakov

2016-04-27 13:07 GMT+03:00 Yakov Zhdanov <[hidden email]>:

> Vlad, not yet, unfortunately. I will try to do today.
>
> --Yakov
>
> 2016-04-27 11:40 GMT+03:00 Vladisav Jelisavcic <[hidden email]>:
>
>> Yakov,
>>
>> did you had time to do another review round of ignite-642?
>>
>> Thanks!
>>
>> On Fri, Apr 15, 2016 at 3:53 PM, Vladisav Jelisavcic <[hidden email]
>> >
>> wrote:
>>
>> > Yakov,
>> >
>> > I've finished the initialization tests for ignite-642 (and moved
>> > serialization test from GridCacheLockAbstractTest to
>> > IgniteLockAbstractSelfTest).
>> > Please check the commit and let me know if you spot anything else.
>> > Thanks!
>> >
>> > On Fri, Apr 15, 2016 at 10:11 AM, Vladisav Jelisavcic <
>> [hidden email]
>> > > wrote:
>> >
>> >> Yakov,
>> >>
>> >> I reviewed the changes in ignite-642 and it looks good to me, but I
>> have
>> >> one question.
>> >> Can you please look at my comment in ignite-642 ticket?
>> >>
>> >> Thanks!
>> >> Vladisav
>> >>
>> >> On Thu, Apr 14, 2016 at 7:51 PM, Vladisav Jelisavcic <
>> [hidden email]
>> >> > wrote:
>> >>
>> >>> Sure, I'll look into it later today, or tomorrow at the latest
>> >>>
>> >>> On Thu, Apr 14, 2016 at 5:53 PM, Yakov Zhdanov <[hidden email]>
>> >>> wrote:
>> >>>
>> >>>> Vlad, please see my changes in ignite-642 and comment in the ticket.
>> >>>>
>> >>>> Alex, can you please take a look at my latest commit as well and
>> provide
>> >>>> comments?
>> >>>>
>> >>>> --Yakov
>> >>>>
>> >>>> 2016-04-12 23:47 GMT+03:00 Yakov Zhdanov <[hidden email]>:
>> >>>>
>> >>>> > Very good points, Alexey. I will look at this tomorrow and finalize
>> >>>> the
>> >>>> > changes.
>> >>>> >
>> >>>> > --Yakov
>> >>>> >
>> >>>> > 2016-04-12 23:41 GMT+03:00 Alexey Goncharuk <
>> >>>> [hidden email]>:
>> >>>> >
>> >>>> >> Guys,
>> >>>> >>
>> >>>> >> I fixed code style a bit and pushed my changes to the branch.
>> >>>> >>
>> >>>> >> Couple of questions:
>> >>>> >>  - I see that some of the Errors caught do not get re-thrown
>> (e.g. if
>> >>>> >> interruptAll flag is set). I believe we should at least re-throw
>> >>>> OOME in
>> >>>> >> any case.
>> >>>> >>  - readResolve method is missing for CacheLockImpl. The current
>> >>>> >> readExternal/writeExternal code uses static stash field. I looked
>> >>>> around
>> >>>> >> in
>> >>>> >> the code and found that IgniteKernal uses localIgnite, while
>> >>>> >> GridCacheAdapter uses stash. Which way is the correct one?
>> >>>> >> ​
>> >>>> >>
>> >>>> >
>> >>>> >
>> >>>>
>> >>>
>> >>>
>> >>
>> >
>>
>
>