IGNITE-4713 : refactoring of GridFinishedFuture and GridFutureAdapter

[Александр Меньшиков]

Hello.

I propose to do refactoring of classes "GridFinishedFuture" and
"GridFutureAdapter". There is field "resFlag" which can equals "ERR = 1" or
"RES = 2". So I can replace it to one "bool haveResult" field.

If there are no objections, I'm ready to proceed. If you find more such
classes, please write about them.

[Alexey Goncharuk]

Alexander,

This change is not applicable for GridFutureAdapter because resFlag can
have 3 values there.

2017-02-16 19:58 GMT+03:00 Александр Меньшиков <[hidden email]>:

> Hello.
>
> I propose to do refactoring of classes "GridFinishedFuture" and
> "GridFutureAdapter". There is field "resFlag" which can equals "ERR = 1" or
> "RES = 2". So I can replace it to one "bool haveResult" field.
>
> If there are no objections, I'm ready to proceed. If you find more such
> classes, please write about them.
>

[Александр Меньшиков]

Alexey,

I see only one place where writes in resFlag:

                if (err != null) {
                    resFlag = ERR;
                    this.res = err;
                }
                else {
                    resFlag = RES;
                    this.res = res;
                }

And the comparison with only two values: "ERR" and "RES". Except "assert
resFlag != 0;". So if this "assert" protect from call "get0" before call
"onDone" I think will be clearer to set some ready flag or use "enum" type.
And throw IllegalStateException if condition is false, because right now
developer will not get clear error massage.

17 февр. 2017 г. 11:34 пользователь "Alexey Goncharuk" <
[hidden email]> написал:

Alexander,

This change is not applicable for GridFutureAdapter because resFlag can
have 3 values there.

2017-02-16 19:58 GMT+03:00 Александр Меньшиков <[hidden email]>:

> Hello.
>
> I propose to do refactoring of classes "GridFinishedFuture" and
> "GridFutureAdapter". There is field "resFlag" which can equals "ERR = 1"
or
> "RES = 2". So I can replace it to one "bool haveResult" field.
>
> If there are no objections, I'm ready to proceed. If you find more such
> classes, please write about them.
>

[V.Pyatkov]

Alexander,

I think, the resFlag will be initiated as 0 (new GridFutureAdapter()), but
1 and 2 states will be acquired on live.


On Fri, Feb 17, 2017 at 1:56 PM, Александр Меньшиков <[hidden email]>
wrote:

> Alexey,
>
> I see only one place where writes in resFlag:
>
>                 if (err != null) {
>                     resFlag = ERR;
>                     this.res = err;
>                 }
>                 else {
>                     resFlag = RES;
>                     this.res = res;
>                 }
>
> And the comparison with only two values: "ERR" and "RES". Except "assert
> resFlag != 0;". So if this "assert" protect from call "get0" before call
> "onDone" I think will be clearer to set some ready flag or use "enum" type.
> And throw IllegalStateException if condition is false, because right now
> developer will not get clear error massage.
>
> 17 февр. 2017 г. 11:34 пользователь "Alexey Goncharuk" <
> [hidden email]> написал:
>
> Alexander,
>
> This change is not applicable for GridFutureAdapter because resFlag can
> have 3 values there.
>
> 2017-02-16 19:58 GMT+03:00 Александр Меньшиков <[hidden email]>:
>
> > Hello.
> >
> > I propose to do refactoring of classes "GridFinishedFuture" and
> > "GridFutureAdapter". There is field "resFlag" which can equals "ERR = 1"
> or
> > "RES = 2". So I can replace it to one "bool haveResult" field.
> >
> > If there are no objections, I'm ready to proceed. If you find more such
> > classes, please write about them.
> >
>

--
Vladislav Pyatkov

[Александр Меньшиков]

Like I said, if "resFlag==0" (of course 0 came from initialization) means
"you still haven't called the method onDone()", better make it clear.


2017-02-17 14:48 GMT+03:00 Vladislav Pyatkov <[hidden email]>:

> Alexander,
>
> I think, the resFlag will be initiated as 0 (new GridFutureAdapter()), but
> 1 and 2 states will be acquired on live.
>
>
> On Fri, Feb 17, 2017 at 1:56 PM, Александр Меньшиков <[hidden email]
> >
> wrote:
>
> > Alexey,
> >
> > I see only one place where writes in resFlag:
> >
> >                 if (err != null) {
> >                     resFlag = ERR;
> >                     this.res = err;
> >                 }
> >                 else {
> >                     resFlag = RES;
> >                     this.res = res;
> >                 }
> >
> > And the comparison with only two values: "ERR" and "RES". Except "assert
> > resFlag != 0;". So if this "assert" protect from call "get0" before call
> > "onDone" I think will be clearer to set some ready flag or use "enum"
> type.
> > And throw IllegalStateException if condition is false, because right now
> > developer will not get clear error massage.
> >
> > 17 февр. 2017 г. 11:34 пользователь "Alexey Goncharuk" <
> > [hidden email]> написал:
> >
> > Alexander,
> >
> > This change is not applicable for GridFutureAdapter because resFlag can
> > have 3 values there.
> >
> > 2017-02-16 19:58 GMT+03:00 Александр Меньшиков <[hidden email]>:
> >
> > > Hello.
> > >
> > > I propose to do refactoring of classes "GridFinishedFuture" and
> > > "GridFutureAdapter". There is field "resFlag" which can equals "ERR =
> 1"
> > or
> > > "RES = 2". So I can replace it to one "bool haveResult" field.
> > >
> > > If there are no objections, I'm ready to proceed. If you find more such
> > > classes, please write about them.
> > >
> >
>
>
>
> --
> Vladislav Pyatkov
>

[Александр Меньшиков]

We can check "onDone" method was called with using getState() method. If
getState()!=0 then  resFlag!=0. Just look at code.

2017-02-17 17:12 GMT+03:00 Александр Меньшиков <[hidden email]>:

> Like I said, if "resFlag==0" (of course 0 came from initialization) means
> "you still haven't called the method onDone()", better make it clear.
>
>
>
> 2017-02-17 14:48 GMT+03:00 Vladislav Pyatkov <[hidden email]>:
>
>> Alexander,
>>
>> I think, the resFlag will be initiated as 0 (new GridFutureAdapter()), but
>> 1 and 2 states will be acquired on live.
>>
>>
>> On Fri, Feb 17, 2017 at 1:56 PM, Александр Меньшиков <
>> [hidden email]>
>> wrote:
>>
>> > Alexey,
>> >
>> > I see only one place where writes in resFlag:
>> >
>> >                 if (err != null) {
>> >                     resFlag = ERR;
>> >                     this.res = err;
>> >                 }
>> >                 else {
>> >                     resFlag = RES;
>> >                     this.res = res;
>> >                 }
>> >
>> > And the comparison with only two values: "ERR" and "RES". Except "assert
>> > resFlag != 0;". So if this "assert" protect from call "get0" before call
>> > "onDone" I think will be clearer to set some ready flag or use "enum"
>> type.
>> > And throw IllegalStateException if condition is false, because right now
>> > developer will not get clear error massage.
>> >
>> > 17 февр. 2017 г. 11:34 пользователь "Alexey Goncharuk" <
>> > [hidden email]> написал:
>> >
>> > Alexander,
>> >
>> > This change is not applicable for GridFutureAdapter because resFlag can
>> > have 3 values there.
>> >
>> > 2017-02-16 19:58 GMT+03:00 Александр Меньшиков <[hidden email]>:
>> >
>> > > Hello.
>> > >
>> > > I propose to do refactoring of classes "GridFinishedFuture" and
>> > > "GridFutureAdapter". There is field "resFlag" which can equals "ERR =
>> 1"
>> > or
>> > > "RES = 2". So I can replace it to one "bool haveResult" field.
>> > >
>> > > If there are no objections, I'm ready to proceed. If you find more
>> such
>> > > classes, please write about them.
>> > >
>> >
>>
>>
>>
>> --
>> Vladislav Pyatkov
>>
>
>

[Александр Меньшиков]

Alexey, Vladislav, are you agree with me, or not? I want to do it faster
and move on.

2017-02-17 17:36 GMT+03:00 Александр Меньшиков <[hidden email]>:

> We can check "onDone" method was called with using getState() method. If
> getState()!=0 then  resFlag!=0. Just look at code.
>
>
> 2017-02-17 17:12 GMT+03:00 Александр Меньшиков <[hidden email]>:
>
>> Like I said, if "resFlag==0" (of course 0 came from initialization) means
>> "you still haven't called the method onDone()", better make it clear.
>>
>>
>>
>> 2017-02-17 14:48 GMT+03:00 Vladislav Pyatkov <[hidden email]>:
>>
>>> Alexander,
>>>
>>> I think, the resFlag will be initiated as 0 (new GridFutureAdapter()),
>>> but
>>> 1 and 2 states will be acquired on live.
>>>
>>>
>>> On Fri, Feb 17, 2017 at 1:56 PM, Александр Меньшиков <
>>> [hidden email]>
>>> wrote:
>>>
>>> > Alexey,
>>> >
>>> > I see only one place where writes in resFlag:
>>> >
>>> >                 if (err != null) {
>>> >                     resFlag = ERR;
>>> >                     this.res = err;
>>> >                 }
>>> >                 else {
>>> >                     resFlag = RES;
>>> >                     this.res = res;
>>> >                 }
>>> >
>>> > And the comparison with only two values: "ERR" and "RES". Except
>>> "assert
>>> > resFlag != 0;". So if this "assert" protect from call "get0" before
>>> call
>>> > "onDone" I think will be clearer to set some ready flag or use "enum"
>>> type.
>>> > And throw IllegalStateException if condition is false, because right
>>> now
>>> > developer will not get clear error massage.
>>> >
>>> > 17 февр. 2017 г. 11:34 пользователь "Alexey Goncharuk" <
>>> > [hidden email]> написал:
>>> >
>>> > Alexander,
>>> >
>>> > This change is not applicable for GridFutureAdapter because resFlag can
>>> > have 3 values there.
>>> >
>>> > 2017-02-16 19:58 GMT+03:00 Александр Меньшиков <[hidden email]>:
>>> >
>>> > > Hello.
>>> > >
>>> > > I propose to do refactoring of classes "GridFinishedFuture" and
>>> > > "GridFutureAdapter". There is field "resFlag" which can equals "ERR
>>> = 1"
>>> > or
>>> > > "RES = 2". So I can replace it to one "bool haveResult" field.
>>> > >
>>> > > If there are no objections, I'm ready to proceed. If you find more
>>> such
>>> > > classes, please write about them.
>>> > >
>>> >
>>>
>>>
>>>
>>> --
>>> Vladislav Pyatkov
>>>
>>
>>
>

[Александр Меньшиков]

Sorry, I mean I want to do fast work on this issue (not make code faster).
Your code is strange. You can see my view in my local temp PR:

https://github.com/SharplEr/ignite/pull/4/files

This is what I'm meaning.

I suppose "volatile" is not need here, because right now if
GridFutureAdapter#onDone() called in another thread than
GridFutureAdapter#get0, it will produce AssertionError.



2017-02-28 16:57 GMT+03:00 Vladislav Pyatkov <[hidden email]>:

> Alexander,
>
> Are you sure, which it will be faster?
> The condition
>
> resFlag == RES and resFlag == ERR
>
> should be more complicated... like something this:
>
> getState() != 0 &&  !resFlag and getState() != 0 &&  resFlag
>
> But unlike getState(), restFag is not volatile...
>
> On Tue, Feb 28, 2017 at 4:26 PM, Александр Меньшиков <[hidden email]
> > wrote:
>
>> Alexey, Vladislav, are you agree with me, or not? I want to do it faster
>> and move on.
>>
>> 2017-02-17 17:36 GMT+03:00 Александр Меньшиков <[hidden email]>:
>>
>>> We can check "onDone" method was called with using getState() method. If
>>> getState()!=0 then  resFlag!=0. Just look at code.
>>>
>>>
>>> 2017-02-17 17:12 GMT+03:00 Александр Меньшиков <[hidden email]>:
>>>
>>>> Like I said, if "resFlag==0" (of course 0 came from initialization)
>>>> means "you still haven't called the method onDone()", better make it clear.
>>>>
>>>>
>>>>
>>>> 2017-02-17 14:48 GMT+03:00 Vladislav Pyatkov <[hidden email]>:
>>>>
>>>>> Alexander,
>>>>>
>>>>> I think, the resFlag will be initiated as 0 (new GridFutureAdapter()),
>>>>> but
>>>>> 1 and 2 states will be acquired on live.
>>>>>
>>>>>
>>>>> On Fri, Feb 17, 2017 at 1:56 PM, Александр Меньшиков <
>>>>> [hidden email]>
>>>>> wrote:
>>>>>
>>>>> > Alexey,
>>>>> >
>>>>> > I see only one place where writes in resFlag:
>>>>> >
>>>>> >                 if (err != null) {
>>>>> >                     resFlag = ERR;
>>>>> >                     this.res = err;
>>>>> >                 }
>>>>> >                 else {
>>>>> >                     resFlag = RES;
>>>>> >                     this.res = res;
>>>>> >                 }
>>>>> >
>>>>> > And the comparison with only two values: "ERR" and "RES". Except
>>>>> "assert
>>>>> > resFlag != 0;". So if this "assert" protect from call "get0" before
>>>>> call
>>>>> > "onDone" I think will be clearer to set some ready flag or use
>>>>> "enum" type.
>>>>> > And throw IllegalStateException if condition is false, because right
>>>>> now
>>>>> > developer will not get clear error massage.
>>>>> >
>>>>> > 17 февр. 2017 г. 11:34 пользователь "Alexey Goncharuk" <
>>>>> > [hidden email]> написал:
>>>>> >
>>>>> > Alexander,
>>>>> >
>>>>> > This change is not applicable for GridFutureAdapter because resFlag
>>>>> can
>>>>> > have 3 values there.
>>>>> >
>>>>> > 2017-02-16 19:58 GMT+03:00 Александр Меньшиков <[hidden email]
>>>>> >:
>>>>> >
>>>>> > > Hello.
>>>>> > >
>>>>> > > I propose to do refactoring of classes "GridFinishedFuture" and
>>>>> > > "GridFutureAdapter". There is field "resFlag" which can equals
>>>>> "ERR = 1"
>>>>> > or
>>>>> > > "RES = 2". So I can replace it to one "bool haveResult" field.
>>>>> > >
>>>>> > > If there are no objections, I'm ready to proceed. If you find more
>>>>> such
>>>>> > > classes, please write about them.
>>>>> > >
>>>>> >
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Vladislav Pyatkov
>>>>>
>>>>
>>>>
>>>
>>
>
>
> --
> Vladislav Pyatkov
>

[yzhdanov]

Alexander,

Can you please clarify this?

>I suppose "volatile" is not need here, because right now if GridFutureAdapter#onDone()
called in another thread than GridFutureAdapter#get0, it will produce
AssertionError.

1) 99.999% of time GridFutureAdapter#onDone() is called in some other
thread than the one which is frozen on get()
2) What is volatile now which should not be?

Regarding the flag - boolean and byte takes 1 byte in the object. I suppose
some time ago there were 3 options, but now there are only 2. I don't think
we need to change it now.



--Yakov

2017-02-28 17:32 GMT+03:00 Александр Меньшиков <[hidden email]>:

> Sorry, I mean I want to do fast work on this issue (not make code faster).
> Your code is strange. You can see my view in my local temp PR:
>
> https://github.com/SharplEr/ignite/pull/4/files
>
> This is what I'm meaning.
>
> I suppose "volatile" is not need here, because right now if
> GridFutureAdapter#onDone() called in another thread than
> GridFutureAdapter#get0, it will produce AssertionError.
>
>
>
> 2017-02-28 16:57 GMT+03:00 Vladislav Pyatkov <[hidden email]>:
>
> > Alexander,
> >
> > Are you sure, which it will be faster?
> > The condition
> >
> > resFlag == RES and resFlag == ERR
> >
> > should be more complicated... like something this:
> >
> > getState() != 0 &&  !resFlag and getState() != 0 &&  resFlag
> >
> > But unlike getState(), restFag is not volatile...
> >
> > On Tue, Feb 28, 2017 at 4:26 PM, Александр Меньшиков <
> [hidden email]
> > > wrote:
> >
> >> Alexey, Vladislav, are you agree with me, or not? I want to do it faster
> >> and move on.
> >>
> >> 2017-02-17 17:36 GMT+03:00 Александр Меньшиков <[hidden email]>:
> >>
> >>> We can check "onDone" method was called with using getState() method.
> If
> >>> getState()!=0 then  resFlag!=0. Just look at code.
> >>>
> >>>
> >>> 2017-02-17 17:12 GMT+03:00 Александр Меньшиков <[hidden email]>:
> >>>
> >>>> Like I said, if "resFlag==0" (of course 0 came from initialization)
> >>>> means "you still haven't called the method onDone()", better make it
> clear.
> >>>>
> >>>>
> >>>>
> >>>> 2017-02-17 14:48 GMT+03:00 Vladislav Pyatkov <[hidden email]>:
> >>>>
> >>>>> Alexander,
> >>>>>
> >>>>> I think, the resFlag will be initiated as 0 (new
> GridFutureAdapter()),
> >>>>> but
> >>>>> 1 and 2 states will be acquired on live.
> >>>>>
> >>>>>
> >>>>> On Fri, Feb 17, 2017 at 1:56 PM, Александр Меньшиков <
> >>>>> [hidden email]>
> >>>>> wrote:
> >>>>>
> >>>>> > Alexey,
> >>>>> >
> >>>>> > I see only one place where writes in resFlag:
> >>>>> >
> >>>>> >                 if (err != null) {
> >>>>> >                     resFlag = ERR;
> >>>>> >                     this.res = err;
> >>>>> >                 }
> >>>>> >                 else {
> >>>>> >                     resFlag = RES;
> >>>>> >                     this.res = res;
> >>>>> >                 }
> >>>>> >
> >>>>> > And the comparison with only two values: "ERR" and "RES". Except
> >>>>> "assert
> >>>>> > resFlag != 0;". So if this "assert" protect from call "get0" before
> >>>>> call
> >>>>> > "onDone" I think will be clearer to set some ready flag or use
> >>>>> "enum" type.
> >>>>> > And throw IllegalStateException if condition is false, because
> right
> >>>>> now
> >>>>> > developer will not get clear error massage.
> >>>>> >
> >>>>> > 17 февр. 2017 г. 11:34 пользователь "Alexey Goncharuk" <
> >>>>> > [hidden email]> написал:
> >>>>> >
> >>>>> > Alexander,
> >>>>> >
> >>>>> > This change is not applicable for GridFutureAdapter because resFlag
> >>>>> can
> >>>>> > have 3 values there.
> >>>>> >
> >>>>> > 2017-02-16 19:58 GMT+03:00 Александр Меньшиков <
> [hidden email]
> >>>>> >:
> >>>>> >
> >>>>> > > Hello.
> >>>>> > >
> >>>>> > > I propose to do refactoring of classes "GridFinishedFuture" and
> >>>>> > > "GridFutureAdapter". There is field "resFlag" which can equals
> >>>>> "ERR = 1"
> >>>>> > or
> >>>>> > > "RES = 2". So I can replace it to one "bool haveResult" field.
> >>>>> > >
> >>>>> > > If there are no objections, I'm ready to proceed. If you find
> more
> >>>>> such
> >>>>> > > classes, please write about them.
> >>>>> > >
> >>>>> >
> >>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Vladislav Pyatkov
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >
> >
> > --
> > Vladislav Pyatkov
> >
>

[Александр Меньшиков]

First of all I want to replace the "byte resFlag" on the "bool haveResult"
only for aesthetic reasons. I think that use a byte as a boolean flag looks
like C in 1983. And constructions like

return haveResult ? (R)res : null;

more readable unlike

return resFlag == RES ? (R)res : null;

because you don't know how many values the "resFlag" can have.
But it's not big deal, so if community disagrees it's okay.

About volatile:

Class "GridFutureAdapter" uses zero value of the "resFlag" in assert for
check that didn't call the "get0" before the "onDone".
So I suggested to use the "getState()" for this check. But of course it's
work only if "get0" and "onDone" are called in a same thread.
But if threads are different (as you said), we have bug any way because the
"resFlag" is not volatile.
In the GridFutureAdapter we haven't any sync between write operations to
the "resFlag" in the "onDone" and the read operation in the "get0". So the
"get0" in the line "assert resFlag != 0" can throw AssertionError, because
a thread with the "get0" may not see the write operation in "onDone" in
other thread.

So I think in this case we need to add volatile in the
GridFutureAdapter.resFlag, and replace the GridFinishedFuture.resFlag on the
GridFinishedFuture.haveResult (or not if you don't like it).


2017-02-28 19:39 GMT+03:00 Yakov Zhdanov <[hidden email]>:

> Alexander,
>
> Can you please clarify this?
>
> >I suppose "volatile" is not need here, because right now if
> GridFutureAdapter#onDone()
> called in another thread than GridFutureAdapter#get0, it will produce
> AssertionError.
>
> 1) 99.999% of time GridFutureAdapter#onDone() is called in some other
> thread than the one which is frozen on get()
> 2) What is volatile now which should not be?
>
> Regarding the flag - boolean and byte takes 1 byte in the object. I suppose
> some time ago there were 3 options, but now there are only 2. I don't think
> we need to change it now.
>
>
>
> --Yakov
>
> 2017-02-28 17:32 GMT+03:00 Александр Меньшиков <[hidden email]>:
>
> > Sorry, I mean I want to do fast work on this issue (not make code
> faster).
> > Your code is strange. You can see my view in my local temp PR:
> >
> > https://github.com/SharplEr/ignite/pull/4/files
> >
> > This is what I'm meaning.
> >
> > I suppose "volatile" is not need here, because right now if
> > GridFutureAdapter#onDone() called in another thread than
> > GridFutureAdapter#get0, it will produce AssertionError.
> >
> >
> >
> > 2017-02-28 16:57 GMT+03:00 Vladislav Pyatkov <[hidden email]>:
> >
> > > Alexander,
> > >
> > > Are you sure, which it will be faster?
> > > The condition
> > >
> > > resFlag == RES and resFlag == ERR
> > >
> > > should be more complicated... like something this:
> > >
> > > getState() != 0 &&  !resFlag and getState() != 0 &&  resFlag
> > >
> > > But unlike getState(), restFag is not volatile...
> > >
> > > On Tue, Feb 28, 2017 at 4:26 PM, Александр Меньшиков <
> > [hidden email]
> > > > wrote:
> > >
> > >> Alexey, Vladislav, are you agree with me, or not? I want to do it
> faster
> > >> and move on.
> > >>
> > >> 2017-02-17 17:36 GMT+03:00 Александр Меньшиков <[hidden email]
> >:
> > >>
> > >>> We can check "onDone" method was called with using getState() method.
> > If
> > >>> getState()!=0 then  resFlag!=0. Just look at code.
> > >>>
> > >>>
> > >>> 2017-02-17 17:12 GMT+03:00 Александр Меньшиков <[hidden email]
> >:
> > >>>
> > >>>> Like I said, if "resFlag==0" (of course 0 came from initialization)
> > >>>> means "you still haven't called the method onDone()", better make it
> > clear.
> > >>>>
> > >>>>
> > >>>>
> > >>>> 2017-02-17 14:48 GMT+03:00 Vladislav Pyatkov <[hidden email]
> >:
> > >>>>
> > >>>>> Alexander,
> > >>>>>
> > >>>>> I think, the resFlag will be initiated as 0 (new
> > GridFutureAdapter()),
> > >>>>> but
> > >>>>> 1 and 2 states will be acquired on live.
> > >>>>>
> > >>>>>
> > >>>>> On Fri, Feb 17, 2017 at 1:56 PM, Александр Меньшиков <
> > >>>>> [hidden email]>
> > >>>>> wrote:
> > >>>>>
> > >>>>> > Alexey,
> > >>>>> >
> > >>>>> > I see only one place where writes in resFlag:
> > >>>>> >
> > >>>>> >                 if (err != null) {
> > >>>>> >                     resFlag = ERR;
> > >>>>> >                     this.res = err;
> > >>>>> >                 }
> > >>>>> >                 else {
> > >>>>> >                     resFlag = RES;
> > >>>>> >                     this.res = res;
> > >>>>> >                 }
> > >>>>> >
> > >>>>> > And the comparison with only two values: "ERR" and "RES". Except
> > >>>>> "assert
> > >>>>> > resFlag != 0;". So if this "assert" protect from call "get0"
> before
> > >>>>> call
> > >>>>> > "onDone" I think will be clearer to set some ready flag or use
> > >>>>> "enum" type.
> > >>>>> > And throw IllegalStateException if condition is false, because
> > right
> > >>>>> now
> > >>>>> > developer will not get clear error massage.
> > >>>>> >
> > >>>>> > 17 февр. 2017 г. 11:34 пользователь "Alexey Goncharuk" <
> > >>>>> > [hidden email]> написал:
> > >>>>> >
> > >>>>> > Alexander,
> > >>>>> >
> > >>>>> > This change is not applicable for GridFutureAdapter because
> resFlag
> > >>>>> can
> > >>>>> > have 3 values there.
> > >>>>> >
> > >>>>> > 2017-02-16 19:58 GMT+03:00 Александр Меньшиков <
> > [hidden email]
> > >>>>> >:
> > >>>>> >
> > >>>>> > > Hello.
> > >>>>> > >
> > >>>>> > > I propose to do refactoring of classes "GridFinishedFuture" and
> > >>>>> > > "GridFutureAdapter". There is field "resFlag" which can equals
> > >>>>> "ERR = 1"
> > >>>>> > or
> > >>>>> > > "RES = 2". So I can replace it to one "bool haveResult" field.
> > >>>>> > >
> > >>>>> > > If there are no objections, I'm ready to proceed. If you find
> > more
> > >>>>> such
> > >>>>> > > classes, please write about them.
> > >>>>> > >
> > >>>>> >
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> --
> > >>>>> Vladislav Pyatkov
> > >>>>>
> > >>>>
> > >>>>
> > >>>
> > >>
> > >
> > >
> > > --
> > > Vladislav Pyatkov
> > >
> >
>

[Александр Меньшиков]

I'm sorry I didn't notice pair "acquireShared" and "releaseShared". Okay,
well, there are no races. But we still can use "getState()" in "assert"
instead "resFlag", that will allow us use boolean type. But if you think it
will not make code cleaner, then I will close issue.

2017-03-01 14:55 GMT+03:00 Александр Меньшиков <[hidden email]>:

> First of all I want to replace the "byte resFlag" on the "bool haveResult"
> only for aesthetic reasons. I think that use a byte as a boolean flag looks
> like C in 1983. And constructions like
>
> return haveResult ? (R)res : null;
>
> more readable unlike
>
> return resFlag == RES ? (R)res : null;
>
> because you don't know how many values the "resFlag" can have.
> But it's not big deal, so if community disagrees it's okay.
>
> About volatile:
>
> Class "GridFutureAdapter" uses zero value of the "resFlag" in assert for
> check that didn't call the "get0" before the "onDone".
> So I suggested to use the "getState()" for this check. But of course it's
> work only if "get0" and "onDone" are called in a same thread.
> But if threads are different (as you said), we have bug any way because
> the "resFlag" is not volatile.
> In the GridFutureAdapter we haven't any sync between write operations to
> the "resFlag" in the "onDone" and the read operation in the "get0". So
> the "get0" in the line "assert resFlag != 0" can throw AssertionError,
> because a thread with the "get0" may not see the write operation in
> "onDone" in other thread.
>
> So I think in this case we need to add volatile in the
> GridFutureAdapter.resFlag, and replace the GridFinishedFuture.resFlag on the
> GridFinishedFuture.haveResult (or not if you don't like it).
>
>
> 2017-02-28 19:39 GMT+03:00 Yakov Zhdanov <[hidden email]>:
>
>> Alexander,
>>
>> Can you please clarify this?
>>
>> >I suppose "volatile" is not need here, because right now if
>> GridFutureAdapter#onDone()
>> called in another thread than GridFutureAdapter#get0, it will produce
>> AssertionError.
>>
>> 1) 99.999% of time GridFutureAdapter#onDone() is called in some other
>> thread than the one which is frozen on get()
>> 2) What is volatile now which should not be?
>>
>> Regarding the flag - boolean and byte takes 1 byte in the object. I
>> suppose
>> some time ago there were 3 options, but now there are only 2. I don't
>> think
>> we need to change it now.
>>
>>
>>
>> --Yakov
>>
>> 2017-02-28 17:32 GMT+03:00 Александр Меньшиков <[hidden email]>:
>>
>> > Sorry, I mean I want to do fast work on this issue (not make code
>> faster).
>> > Your code is strange. You can see my view in my local temp PR:
>> >
>> > https://github.com/SharplEr/ignite/pull/4/files
>> >
>> > This is what I'm meaning.
>> >
>> > I suppose "volatile" is not need here, because right now if
>> > GridFutureAdapter#onDone() called in another thread than
>> > GridFutureAdapter#get0, it will produce AssertionError.
>> >
>> >
>> >
>> > 2017-02-28 16:57 GMT+03:00 Vladislav Pyatkov <[hidden email]>:
>> >
>> > > Alexander,
>> > >
>> > > Are you sure, which it will be faster?
>> > > The condition
>> > >
>> > > resFlag == RES and resFlag == ERR
>> > >
>> > > should be more complicated... like something this:
>> > >
>> > > getState() != 0 &&  !resFlag and getState() != 0 &&  resFlag
>> > >
>> > > But unlike getState(), restFag is not volatile...
>> > >
>> > > On Tue, Feb 28, 2017 at 4:26 PM, Александр Меньшиков <
>> > [hidden email]
>> > > > wrote:
>> > >
>> > >> Alexey, Vladislav, are you agree with me, or not? I want to do it
>> faster
>> > >> and move on.
>> > >>
>> > >> 2017-02-17 17:36 GMT+03:00 Александр Меньшиков <[hidden email]
>> >:
>> > >>
>> > >>> We can check "onDone" method was called with using getState()
>> method.
>> > If
>> > >>> getState()!=0 then  resFlag!=0. Just look at code.
>> > >>>
>> > >>>
>> > >>> 2017-02-17 17:12 GMT+03:00 Александр Меньшиков <
>> [hidden email]>:
>> > >>>
>> > >>>> Like I said, if "resFlag==0" (of course 0 came from initialization)
>> > >>>> means "you still haven't called the method onDone()", better make
>> it
>> > clear.
>> > >>>>
>> > >>>>
>> > >>>>
>> > >>>> 2017-02-17 14:48 GMT+03:00 Vladislav Pyatkov <[hidden email]
>> >:
>> > >>>>
>> > >>>>> Alexander,
>> > >>>>>
>> > >>>>> I think, the resFlag will be initiated as 0 (new
>> > GridFutureAdapter()),
>> > >>>>> but
>> > >>>>> 1 and 2 states will be acquired on live.
>> > >>>>>
>> > >>>>>
>> > >>>>> On Fri, Feb 17, 2017 at 1:56 PM, Александр Меньшиков <
>> > >>>>> [hidden email]>
>> > >>>>> wrote:
>> > >>>>>
>> > >>>>> > Alexey,
>> > >>>>> >
>> > >>>>> > I see only one place where writes in resFlag:
>> > >>>>> >
>> > >>>>> >                 if (err != null) {
>> > >>>>> >                     resFlag = ERR;
>> > >>>>> >                     this.res = err;
>> > >>>>> >                 }
>> > >>>>> >                 else {
>> > >>>>> >                     resFlag = RES;
>> > >>>>> >                     this.res = res;
>> > >>>>> >                 }
>> > >>>>> >
>> > >>>>> > And the comparison with only two values: "ERR" and "RES". Except
>> > >>>>> "assert
>> > >>>>> > resFlag != 0;". So if this "assert" protect from call "get0"
>> before
>> > >>>>> call
>> > >>>>> > "onDone" I think will be clearer to set some ready flag or use
>> > >>>>> "enum" type.
>> > >>>>> > And throw IllegalStateException if condition is false, because
>> > right
>> > >>>>> now
>> > >>>>> > developer will not get clear error massage.
>> > >>>>> >
>> > >>>>> > 17 февр. 2017 г. 11:34 пользователь "Alexey Goncharuk" <
>> > >>>>> > [hidden email]> написал:
>> > >>>>> >
>> > >>>>> > Alexander,
>> > >>>>> >
>> > >>>>> > This change is not applicable for GridFutureAdapter because
>> resFlag
>> > >>>>> can
>> > >>>>> > have 3 values there.
>> > >>>>> >
>> > >>>>> > 2017-02-16 19:58 GMT+03:00 Александр Меньшиков <
>> > [hidden email]
>> > >>>>> >:
>> > >>>>> >
>> > >>>>> > > Hello.
>> > >>>>> > >
>> > >>>>> > > I propose to do refactoring of classes "GridFinishedFuture"
>> and
>> > >>>>> > > "GridFutureAdapter". There is field "resFlag" which can equals
>> > >>>>> "ERR = 1"
>> > >>>>> > or
>> > >>>>> > > "RES = 2". So I can replace it to one "bool haveResult" field.
>> > >>>>> > >
>> > >>>>> > > If there are no objections, I'm ready to proceed. If you find
>> > more
>> > >>>>> such
>> > >>>>> > > classes, please write about them.
>> > >>>>> > >
>> > >>>>> >
>> > >>>>>
>> > >>>>>
>> > >>>>>
>> > >>>>> --
>> > >>>>> Vladislav Pyatkov
>> > >>>>>
>> > >>>>
>> > >>>>
>> > >>>
>> > >>
>> > >
>> > >
>> > > --
>> > > Vladislav Pyatkov
>> > >
>> >
>>
>
>