Ability to check and completely fill transactions on creation

[Anton Vinogradov-2]

Igniters,

As far as I know we're working on additional 'label' field for transactions
[1].
That's great and will be helpful for customers with huge deploymens to see
reason of each transaction.
But, since 'label' is optional field, there is no way to guarantee it will
be filled.

I'd like to propose an idea of brand new event EVT_USR_TX_CREATED (local
transaction created).

Local listener on such event will allow to guarantee tx's 'label' field
filled, timeout is correct and so on.

Thoughts?

[1] https://issues.apache.org/jira/browse/IGNITE-6827

[Nikolay Izhikov-2]

+1 from me.

Let's have this events!

I think `EVT_USR_TX_*START*` is more convinient name for it.
As far as we have method `IgniteTransctions#txStart`

В Чт, 05/04/2018 в 16:06 +0300, Anton Vinogradov пишет:

> Igniters,
>
> As far as I know we're working on additional 'label' field for transactions
> [1].
> That's great and will be helpful for customers with huge deploymens to see
> reason of each transaction.
> But, since 'label' is optional field, there is no way to guarantee it will
> be filled.
>
> I'd like to propose an idea of brand new event EVT_USR_TX_CREATED (local
> transaction created).
>
> Local listener on such event will allow to guarantee tx's 'label' field
> filled, timeout is correct and so on.
>
> Thoughts?
>
> [1] https://issues.apache.org/jira/browse/IGNITE-6827

[Dmitriy Pavlov]

Hi Igniters,

I also do not see any reasons against this solution. But I have concern
about performance. How can you estimate impact to performance ?

Sincerely,
Dmitriy Pavlov

чт, 5 апр. 2018 г. в 17:20, Nikolay Izhikov <[hidden email]>:

> +1 from me.
>
> Let's have this events!
>
> I think `EVT_USR_TX_*START*` is more convinient name for it.
> As far as we have method `IgniteTransctions#txStart`
>
> В Чт, 05/04/2018 в 16:06 +0300, Anton Vinogradov пишет:
> > Igniters,
> >
> > As far as I know we're working on additional 'label' field for
> transactions
> > [1].
> > That's great and will be helpful for customers with huge deploymens to
> see
> > reason of each transaction.
> > But, since 'label' is optional field, there is no way to guarantee it
> will
> > be filled.
> >
> > I'd like to propose an idea of brand new event EVT_USR_TX_CREATED (local
> > transaction created).
> >
> > Local listener on such event will allow to guarantee tx's 'label' field
> > filled, timeout is correct and so on.
> >
> > Thoughts?
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-6827

[Alexey Kuznetsov]

How about to set label name with some useful info if user does not provide
custom name?

For example thread name + global counter?

Thoughts?

--
Alexey Kuznetsov

[dmagda]

Guys,

Sorry for a dumb question but how the user is expected to use this event?

--
Denis

On Thu, Apr 5, 2018 at 6:06 AM, Anton Vinogradov <[hidden email]> wrote:

> Igniters,
>
> As far as I know we're working on additional 'label' field for transactions
> [1].
> That's great and will be helpful for customers with huge deploymens to see
> reason of each transaction.
> But, since 'label' is optional field, there is no way to guarantee it will
> be filled.
>
> I'd like to propose an idea of brand new event EVT_USR_TX_CREATED (local
> transaction created).
>
> Local listener on such event will allow to guarantee tx's 'label' field
> filled, timeout is correct and so on.
>
> Thoughts?
>
> [1] https://issues.apache.org/jira/browse/IGNITE-6827
>

[Anton Vinogradov-2]

 >> But I have concern
>> about performance. How can you estimate impact to performance ?
We have to benchmark result.

>> How about to set label name with some useful info if user does not
provide
>> custom name?
You can set custom listener which will do that

>>  For example thread name + global counter?
Or even full stacktrace

>>  how the user is expected to use this event?
Event will be used to validate tx on creation.
Since listner will be invoked at same thread (Am I right?) it will have all
requred info for validation.


2018-04-05 22:06 GMT+03:00 Denis Magda <[hidden email]>:

> Guys,
>
> Sorry for a dumb question but how the user is expected to use this event?
>
> --
> Denis
>
> On Thu, Apr 5, 2018 at 6:06 AM, Anton Vinogradov <[hidden email]> wrote:
>
> > Igniters,
> >
> > As far as I know we're working on additional 'label' field for
> transactions
> > [1].
> > That's great and will be helpful for customers with huge deploymens to
> see
> > reason of each transaction.
> > But, since 'label' is optional field, there is no way to guarantee it
> will
> > be filled.
> >
> > I'd like to propose an idea of brand new event EVT_USR_TX_CREATED (local
> > transaction created).
> >
> > Local listener on such event will allow to guarantee tx's 'label' field
> > filled, timeout is correct and so on.
> >
> > Thoughts?
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-6827
> >
>

[Anton Vinogradov-2]

Igniters,

Fix is ready.
Code checked by Nikolay Izhikov, all issues fixed
Code benchmarked by Ilya Suntsov, no drop found.

Fix *affects public API*, so I'm proposing everyone interested check it.
Issue: https://issues.apache.org/jira/browse/IGNITE-8446
PR: https://github.com/apache/ignite/pull/4036/files

Public API changes:
New EvenType:

EVT_TX_STARTED = 129.

New Event:

TransactionEvent extends EventAdapter {
   private IgniteInternalTx tx;
}

In case there are no objection I'll merge the changes soon.

пт, 6 апр. 2018 г. в 14:12, Anton Vinogradov <[hidden email]>:

> >> But I have concern
> >> about performance. How can you estimate impact to performance ?
> We have to benchmark result.
>
> >> How about to set label name with some useful info if user does not
> provide
> >> custom name?
> You can set custom listener which will do that
>
> >>  For example thread name + global counter?
> Or even full stacktrace
>
> >>  how the user is expected to use this event?
> Event will be used to validate tx on creation.
> Since listner will be invoked at same thread (Am I right?) it will have
> all requred info for validation.
>
>
> 2018-04-05 22:06 GMT+03:00 Denis Magda <[hidden email]>:
>
>> Guys,
>>
>> Sorry for a dumb question but how the user is expected to use this event?
>>
>> --
>> Denis
>>
>> On Thu, Apr 5, 2018 at 6:06 AM, Anton Vinogradov <[hidden email]> wrote:
>>
>> > Igniters,
>> >
>> > As far as I know we're working on additional 'label' field for
>> transactions
>> > [1].
>> > That's great and will be helpful for customers with huge deploymens to
>> see
>> > reason of each transaction.
>> > But, since 'label' is optional field, there is no way to guarantee it
>> will
>> > be filled.
>> >
>> > I'd like to propose an idea of brand new event EVT_USR_TX_CREATED (local
>> > transaction created).
>> >
>> > Local listener on such event will allow to guarantee tx's 'label' field
>> > filled, timeout is correct and so on.
>> >
>> > Thoughts?
>> >
>> > [1] https://issues.apache.org/jira/browse/IGNITE-6827
>> >
>>
>
>

[yzhdanov]

Anton, I have objections. Please do not merge unless we agree on details.

1. You use internal API in public event, i.e. you cannot have user
accessing to IgniteInternalTx instance through TxEvent.
2. Throwing runtime errors from listener is not documented and I doubt if
it can be fully supported in the pattern you use in TxLabelTest. After
looking at the mentioned test user may think that throwing runtime error
when notified on new node join may prohibit new node joining which is not
true. Do you have any example in Ignite when throwing exception from
listener is valid and documented.
3. TxLabelTest is not included into any suite.
4. EVT_TX_STARTED - does not seems to be a proper and descriptive name

I think that we should think about some other solution instead of altering
event sub-system.

Also I want to ask everyone in community to request explicit approval from
community members before changing anything in transactional logic.

Thanks!

--Yakov

[Dmitriy Pavlov]

Hi Yakov,

I've checked this code and IgniteCacheTestSuite6 includes TxLabelTest, so
point 3 can be considered as solved.

Sincerely,
Dmitriy Pavlov

пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <[hidden email]>:

> Anton, I have objections. Please do not merge unless we agree on details.
>
> 1. You use internal API in public event, i.e. you cannot have user
> accessing to IgniteInternalTx instance through TxEvent.
> 2. Throwing runtime errors from listener is not documented and I doubt if
> it can be fully supported in the pattern you use in TxLabelTest. After
> looking at the mentioned test user may think that throwing runtime error
> when notified on new node join may prohibit new node joining which is not
> true. Do you have any example in Ignite when throwing exception from
> listener is valid and documented.
> 3. TxLabelTest is not included into any suite.
> 4. EVT_TX_STARTED - does not seems to be a proper and descriptive name
>
> I think that we should think about some other solution instead of altering
> event sub-system.
>
> Also I want to ask everyone in community to request explicit approval from
> community members before changing anything in transactional logic.
>
> Thanks!
>
> --Yakov
>

[yzhdanov]

Ok, then it probably might have been created before this PR. Anyway, I
would not bother too much about pt 3.

--Yakov

2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <[hidden email]>:

> Hi Yakov,
>
> I've checked this code and IgniteCacheTestSuite6 includes TxLabelTest, so
> point 3 can be considered as solved.
>
> Sincerely,
> Dmitriy Pavlov
>
> пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <[hidden email]>:
>
> > Anton, I have objections. Please do not merge unless we agree on details.
> >
> > 1. You use internal API in public event, i.e. you cannot have user
> > accessing to IgniteInternalTx instance through TxEvent.
> > 2. Throwing runtime errors from listener is not documented and I doubt if
> > it can be fully supported in the pattern you use in TxLabelTest. After
> > looking at the mentioned test user may think that throwing runtime error
> > when notified on new node join may prohibit new node joining which is not
> > true. Do you have any example in Ignite when throwing exception from
> > listener is valid and documented.
> > 3. TxLabelTest is not included into any suite.
> > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive name
> >
> > I think that we should think about some other solution instead of
> altering
> > event sub-system.
> >
> > Also I want to ask everyone in community to request explicit approval
> from
> > community members before changing anything in transactional logic.
> >
> > Thanks!
> >
> > --Yakov
> >
>

[Anton Vinogradov-2]

Yakov, thank's for deep check.

>> I think that we should think about some other solution instead of
altering
>> event sub-system.

Thank's to your comments now I see that solution is not perfect.

How about to create

interface TransactionsValidator {
   boolean validate(IgniteTransactions tx){
      ...
   }
}

and add it's setter to IgniteConfiguration?

icfg.setTransactionsValidator(new CustomTransactionsValidator(...));

In that case we'll gain easy and proper solution allows to check
transaction configuration before real tx creation.

It will be necessary to add some getters to IgniteTransactions:
- label()
- timeout()
- concurrency() (optional)
- isolation() (optional)
- txSize() (optional)

Thoughts?

пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <[hidden email]>:

> Ok, then it probably might have been created before this PR. Anyway, I
> would not bother too much about pt 3.
>
> --Yakov
>
> 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <[hidden email]>:
>
> > Hi Yakov,
> >
> > I've checked this code and IgniteCacheTestSuite6 includes TxLabelTest, so
> > point 3 can be considered as solved.
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <[hidden email]>:
> >
> > > Anton, I have objections. Please do not merge unless we agree on
> details.
> > >
> > > 1. You use internal API in public event, i.e. you cannot have user
> > > accessing to IgniteInternalTx instance through TxEvent.
> > > 2. Throwing runtime errors from listener is not documented and I doubt
> if
> > > it can be fully supported in the pattern you use in TxLabelTest. After
> > > looking at the mentioned test user may think that throwing runtime
> error
> > > when notified on new node join may prohibit new node joining which is
> not
> > > true. Do you have any example in Ignite when throwing exception from
> > > listener is valid and documented.
> > > 3. TxLabelTest is not included into any suite.
> > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive name
> > >
> > > I think that we should think about some other solution instead of
> > altering
> > > event sub-system.
> > >
> > > Also I want to ask everyone in community to request explicit approval
> > from
> > > community members before changing anything in transactional logic.
> > >
> > > Thanks!
> > >
> > > --Yakov
> > >
> >
>

[dsetrakyan]

Anton,

The change looks very questionable. We cannot be adding configuration
validators for every piece of Ignite API. What is it you are trying to
achieve?

D.

On Mon, May 21, 2018 at 7:22 AM, Anton Vinogradov <[hidden email]> wrote:

> Yakov, thank's for deep check.
>
> >> I think that we should think about some other solution instead of
> altering
> >> event sub-system.
>
> Thank's to your comments now I see that solution is not perfect.
>
> How about to create
>
> interface TransactionsValidator {
>    boolean validate(IgniteTransactions tx){
>       ...
>    }
> }
>
> and add it's setter to IgniteConfiguration?
>
> icfg.setTransactionsValidator(new CustomTransactionsValidator(...));
>
> In that case we'll gain easy and proper solution allows to check
> transaction configuration before real tx creation.
>
> It will be necessary to add some getters to IgniteTransactions:
> - label()
> - timeout()
> - concurrency() (optional)
> - isolation() (optional)
> - txSize() (optional)
>
> Thoughts?
>
> пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <[hidden email]>:
>
> > Ok, then it probably might have been created before this PR. Anyway, I
> > would not bother too much about pt 3.
> >
> > --Yakov
> >
> > 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <[hidden email]>:
> >
> > > Hi Yakov,
> > >
> > > I've checked this code and IgniteCacheTestSuite6 includes TxLabelTest,
> so
> > > point 3 can be considered as solved.
> > >
> > > Sincerely,
> > > Dmitriy Pavlov
> > >
> > > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <[hidden email]>:
> > >
> > > > Anton, I have objections. Please do not merge unless we agree on
> > details.
> > > >
> > > > 1. You use internal API in public event, i.e. you cannot have user
> > > > accessing to IgniteInternalTx instance through TxEvent.
> > > > 2. Throwing runtime errors from listener is not documented and I
> doubt
> > if
> > > > it can be fully supported in the pattern you use in TxLabelTest.
> After
> > > > looking at the mentioned test user may think that throwing runtime
> > error
> > > > when notified on new node join may prohibit new node joining which is
> > not
> > > > true. Do you have any example in Ignite when throwing exception from
> > > > listener is valid and documented.
> > > > 3. TxLabelTest is not included into any suite.
> > > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive
> name
> > > >
> > > > I think that we should think about some other solution instead of
> > > altering
> > > > event sub-system.
> > > >
> > > > Also I want to ask everyone in community to request explicit approval
> > > from
> > > > community members before changing anything in transactional logic.
> > > >
> > > > Thanks!
> > > >
> > > > --Yakov
> > > >
> > >
> >
>

[Anton Vinogradov-2]

Dmitriy,

Main idea is to restrict transaction creation in case label or timeout are
not set.
But there can be variations, for example label should fit some regexp or
timeout should be between 30 and 5000 ms.

That's not easy to restrict this at user code when you have dozens of
libraries written by different people in one system using one ignite
cluster.
That solution based on idea of TopologyValidator when you have no chances
to use cluster in case something wrong with nodes.
So, any client should have no chances to create transaction not suitable
for this ignite cluster.

пн, 21 мая 2018 г. в 17:48, Dmitriy Setrakyan <[hidden email]>:

> Anton,
>
> The change looks very questionable. We cannot be adding configuration
> validators for every piece of Ignite API. What is it you are trying to
> achieve?
>
> D.
>
> On Mon, May 21, 2018 at 7:22 AM, Anton Vinogradov <[hidden email]> wrote:
>
> > Yakov, thank's for deep check.
> >
> > >> I think that we should think about some other solution instead of
> > altering
> > >> event sub-system.
> >
> > Thank's to your comments now I see that solution is not perfect.
> >
> > How about to create
> >
> > interface TransactionsValidator {
> >    boolean validate(IgniteTransactions tx){
> >       ...
> >    }
> > }
> >
> > and add it's setter to IgniteConfiguration?
> >
> > icfg.setTransactionsValidator(new CustomTransactionsValidator(...));
> >
> > In that case we'll gain easy and proper solution allows to check
> > transaction configuration before real tx creation.
> >
> > It will be necessary to add some getters to IgniteTransactions:
> > - label()
> > - timeout()
> > - concurrency() (optional)
> > - isolation() (optional)
> > - txSize() (optional)
> >
> > Thoughts?
> >
> > пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <[hidden email]>:
> >
> > > Ok, then it probably might have been created before this PR. Anyway, I
> > > would not bother too much about pt 3.
> > >
> > > --Yakov
> > >
> > > 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <[hidden email]>:
> > >
> > > > Hi Yakov,
> > > >
> > > > I've checked this code and IgniteCacheTestSuite6 includes
> TxLabelTest,
> > so
> > > > point 3 can be considered as solved.
> > > >
> > > > Sincerely,
> > > > Dmitriy Pavlov
> > > >
> > > > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <[hidden email]>:
> > > >
> > > > > Anton, I have objections. Please do not merge unless we agree on
> > > details.
> > > > >
> > > > > 1. You use internal API in public event, i.e. you cannot have user
> > > > > accessing to IgniteInternalTx instance through TxEvent.
> > > > > 2. Throwing runtime errors from listener is not documented and I
> > doubt
> > > if
> > > > > it can be fully supported in the pattern you use in TxLabelTest.
> > After
> > > > > looking at the mentioned test user may think that throwing runtime
> > > error
> > > > > when notified on new node join may prohibit new node joining which
> is
> > > not
> > > > > true. Do you have any example in Ignite when throwing exception
> from
> > > > > listener is valid and documented.
> > > > > 3. TxLabelTest is not included into any suite.
> > > > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive
> > name
> > > > >
> > > > > I think that we should think about some other solution instead of
> > > > altering
> > > > > event sub-system.
> > > > >
> > > > > Also I want to ask everyone in community to request explicit
> approval
> > > > from
> > > > > community members before changing anything in transactional logic.
> > > > >
> > > > > Thanks!
> > > > >
> > > > > --Yakov
> > > > >
> > > >
> > >
> >
>

[Anton Vinogradov-2]

One more case is to analize and log tx's creators info without tx creation
restriction.
This is very important feature on huge system trial period when you may
want to check who creates tx, tx content and configuration.
For example you may want to log stacktrace for any tx without meta or with
empty timeout.
This will allow you to find a team responsible for that and make sure that
they will fix their code.

пн, 21 мая 2018 г. в 18:14, Anton Vinogradov <[hidden email]>:

> Dmitriy,
>
> Main idea is to restrict transaction creation in case label or timeout are
> not set.
> But there can be variations, for example label should fit some regexp or
> timeout should be between 30 and 5000 ms.
>
> That's not easy to restrict this at user code when you have dozens of
> libraries written by different people in one system using one ignite
> cluster.
> That solution based on idea of TopologyValidator when you have no chances
> to use cluster in case something wrong with nodes.
> So, any client should have no chances to create transaction not suitable
> for this ignite cluster.
>
> пн, 21 мая 2018 г. в 17:48, Dmitriy Setrakyan <[hidden email]>:
>
>> Anton,
>>
>> The change looks very questionable. We cannot be adding configuration
>> validators for every piece of Ignite API. What is it you are trying to
>> achieve?
>>
>> D.
>>
>> On Mon, May 21, 2018 at 7:22 AM, Anton Vinogradov <[hidden email]> wrote:
>>
>> > Yakov, thank's for deep check.
>> >
>> > >> I think that we should think about some other solution instead of
>> > altering
>> > >> event sub-system.
>> >
>> > Thank's to your comments now I see that solution is not perfect.
>> >
>> > How about to create
>> >
>> > interface TransactionsValidator {
>> >    boolean validate(IgniteTransactions tx){
>> >       ...
>> >    }
>> > }
>> >
>> > and add it's setter to IgniteConfiguration?
>> >
>> > icfg.setTransactionsValidator(new CustomTransactionsValidator(...));
>> >
>> > In that case we'll gain easy and proper solution allows to check
>> > transaction configuration before real tx creation.
>> >
>> > It will be necessary to add some getters to IgniteTransactions:
>> > - label()
>> > - timeout()
>> > - concurrency() (optional)
>> > - isolation() (optional)
>> > - txSize() (optional)
>> >
>> > Thoughts?
>> >
>> > пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <[hidden email]>:
>> >
>> > > Ok, then it probably might have been created before this PR. Anyway, I
>> > > would not bother too much about pt 3.
>> > >
>> > > --Yakov
>> > >
>> > > 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <[hidden email]>:
>> > >
>> > > > Hi Yakov,
>> > > >
>> > > > I've checked this code and IgniteCacheTestSuite6 includes
>> TxLabelTest,
>> > so
>> > > > point 3 can be considered as solved.
>> > > >
>> > > > Sincerely,
>> > > > Dmitriy Pavlov
>> > > >
>> > > > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <[hidden email]>:
>> > > >
>> > > > > Anton, I have objections. Please do not merge unless we agree on
>> > > details.
>> > > > >
>> > > > > 1. You use internal API in public event, i.e. you cannot have user
>> > > > > accessing to IgniteInternalTx instance through TxEvent.
>> > > > > 2. Throwing runtime errors from listener is not documented and I
>> > doubt
>> > > if
>> > > > > it can be fully supported in the pattern you use in TxLabelTest.
>> > After
>> > > > > looking at the mentioned test user may think that throwing runtime
>> > > error
>> > > > > when notified on new node join may prohibit new node joining
>> which is
>> > > not
>> > > > > true. Do you have any example in Ignite when throwing exception
>> from
>> > > > > listener is valid and documented.
>> > > > > 3. TxLabelTest is not included into any suite.
>> > > > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive
>> > name
>> > > > >
>> > > > > I think that we should think about some other solution instead of
>> > > > altering
>> > > > > event sub-system.
>> > > > >
>> > > > > Also I want to ask everyone in community to request explicit
>> approval
>> > > > from
>> > > > > community members before changing anything in transactional logic.
>> > > > >
>> > > > > Thanks!
>> > > > >
>> > > > > --Yakov
>> > > > >
>> > > >
>> > >
>> >
>>
>

[Anton Vinogradov-2]

Dmitriy, Yakov

Are there any objections to updated design taking into account the comments
I provided?

пн, 21 мая 2018 г. в 18:49, Anton Vinogradov <[hidden email]>:

> One more case is to analize and log tx's creators info without tx creation
> restriction.
> This is very important feature on huge system trial period when you may
> want to check who creates tx, tx content and configuration.
> For example you may want to log stacktrace for any tx without meta or with
> empty timeout.
> This will allow you to find a team responsible for that and make sure that
> they will fix their code.
>
> пн, 21 мая 2018 г. в 18:14, Anton Vinogradov <[hidden email]>:
>
>> Dmitriy,
>>
>> Main idea is to restrict transaction creation in case label or timeout
>> are not set.
>> But there can be variations, for example label should fit some regexp or
>> timeout should be between 30 and 5000 ms.
>>
>> That's not easy to restrict this at user code when you have dozens of
>> libraries written by different people in one system using one ignite
>> cluster.
>> That solution based on idea of TopologyValidator when you have no chances
>> to use cluster in case something wrong with nodes.
>> So, any client should have no chances to create transaction not suitable
>> for this ignite cluster.
>>
>> пн, 21 мая 2018 г. в 17:48, Dmitriy Setrakyan <[hidden email]>:
>>
>>> Anton,
>>>
>>> The change looks very questionable. We cannot be adding configuration
>>> validators for every piece of Ignite API. What is it you are trying to
>>> achieve?
>>>
>>> D.
>>>
>>> On Mon, May 21, 2018 at 7:22 AM, Anton Vinogradov <[hidden email]> wrote:
>>>
>>> > Yakov, thank's for deep check.
>>> >
>>> > >> I think that we should think about some other solution instead of
>>> > altering
>>> > >> event sub-system.
>>> >
>>> > Thank's to your comments now I see that solution is not perfect.
>>> >
>>> > How about to create
>>> >
>>> > interface TransactionsValidator {
>>> >    boolean validate(IgniteTransactions tx){
>>> >       ...
>>> >    }
>>> > }
>>> >
>>> > and add it's setter to IgniteConfiguration?
>>> >
>>> > icfg.setTransactionsValidator(new CustomTransactionsValidator(...));
>>> >
>>> > In that case we'll gain easy and proper solution allows to check
>>> > transaction configuration before real tx creation.
>>> >
>>> > It will be necessary to add some getters to IgniteTransactions:
>>> > - label()
>>> > - timeout()
>>> > - concurrency() (optional)
>>> > - isolation() (optional)
>>> > - txSize() (optional)
>>> >
>>> > Thoughts?
>>> >
>>> > пн, 21 мая 2018 г. в 16:31, Yakov Zhdanov <[hidden email]>:
>>> >
>>> > > Ok, then it probably might have been created before this PR. Anyway,
>>> I
>>> > > would not bother too much about pt 3.
>>> > >
>>> > > --Yakov
>>> > >
>>> > > 2018-05-21 16:15 GMT+03:00 Dmitry Pavlov <[hidden email]>:
>>> > >
>>> > > > Hi Yakov,
>>> > > >
>>> > > > I've checked this code and IgniteCacheTestSuite6 includes
>>> TxLabelTest,
>>> > so
>>> > > > point 3 can be considered as solved.
>>> > > >
>>> > > > Sincerely,
>>> > > > Dmitriy Pavlov
>>> > > >
>>> > > > пн, 21 мая 2018 г. в 16:05, Yakov Zhdanov <[hidden email]>:
>>> > > >
>>> > > > > Anton, I have objections. Please do not merge unless we agree on
>>> > > details.
>>> > > > >
>>> > > > > 1. You use internal API in public event, i.e. you cannot have
>>> user
>>> > > > > accessing to IgniteInternalTx instance through TxEvent.
>>> > > > > 2. Throwing runtime errors from listener is not documented and I
>>> > doubt
>>> > > if
>>> > > > > it can be fully supported in the pattern you use in TxLabelTest.
>>> > After
>>> > > > > looking at the mentioned test user may think that throwing
>>> runtime
>>> > > error
>>> > > > > when notified on new node join may prohibit new node joining
>>> which is
>>> > > not
>>> > > > > true. Do you have any example in Ignite when throwing exception
>>> from
>>> > > > > listener is valid and documented.
>>> > > > > 3. TxLabelTest is not included into any suite.
>>> > > > > 4. EVT_TX_STARTED - does not seems to be a proper and descriptive
>>> > name
>>> > > > >
>>> > > > > I think that we should think about some other solution instead of
>>> > > > altering
>>> > > > > event sub-system.
>>> > > > >
>>> > > > > Also I want to ask everyone in community to request explicit
>>> approval
>>> > > > from
>>> > > > > community members before changing anything in transactional
>>> logic.
>>> > > > >
>>> > > > > Thanks!
>>> > > > >
>>> > > > > --Yakov
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>

[dsetrakyan]

On Wed, May 23, 2018 at 4:08 AM, Anton Vinogradov <[hidden email]> wrote:

> Dmitriy, Yakov
>
> Are there any objections to updated design taking into account the comments
> I provided?
>

Anton, I do not like an additional validator. I think you can accomplish
the same with a transaction event. You just need to design it more cleanly,
incorporating the feedback from Yakov.

[Anton Vinogradov-2]

Dmitriy,

Yakov is against the solution based on event sub-system
>> I think that we should think about some other solution instead of
altering
>> event sub-system.

Also, I checked is there any chances to fix all the issues found by Yakov
and see that solution becomes overcomplicated in that case.
That's why I'm proposing this lightweight solution.

As for me it's a good idea to have such validator since that's a common
problem at huge deployments when more than one team have access to Ignite
cluster and there is no other way to setup tx cretion rules.

Yakov,

Could you please share your thoughts on that?


чт, 24 мая 2018 г. в 8:58, Dmitriy Setrakyan <[hidden email]>:

> On Wed, May 23, 2018 at 4:08 AM, Anton Vinogradov <[hidden email]> wrote:
>
> > Dmitriy, Yakov
> >
> > Are there any objections to updated design taking into account the
> comments
> > I provided?
> >
>
> Anton, I do not like an additional validator. I think you can accomplish
> the same with a transaction event. You just need to design it more cleanly,
> incorporating the feedback from Yakov.
>

[dsetrakyan]

Anton, why do you need to *alter* event sub-system to introduce a new
event? Yakov's issue was that you propagated private interface to public
API, which is bad of course. Come up with a clean design and it will be
accepted.

My problem with TransactionValidator is that it only solves a small problem
for transactions. If we do that, then we will have to add cache validators,
compute validators, etc, etc, etc. That is why we either should use the
existing event subsystem or come up with a holistic design that will work
across the whole project.

D.

On Thu, May 24, 2018 at 1:38 AM, Anton Vinogradov <[hidden email]> wrote:

> Dmitriy,
>
> Yakov is against the solution based on event sub-system
> >> I think that we should think about some other solution instead of
> altering
> >> event sub-system.
>
> Also, I checked is there any chances to fix all the issues found by Yakov
> and see that solution becomes overcomplicated in that case.
> That's why I'm proposing this lightweight solution.
>
> As for me it's a good idea to have such validator since that's a common
> problem at huge deployments when more than one team have access to Ignite
> cluster and there is no other way to setup tx cretion rules.
>
> Yakov,
>
> Could you please share your thoughts on that?
>
>
> чт, 24 мая 2018 г. в 8:58, Dmitriy Setrakyan <[hidden email]>:
>
> > On Wed, May 23, 2018 at 4:08 AM, Anton Vinogradov <[hidden email]> wrote:
> >
> > > Dmitriy, Yakov
> > >
> > > Are there any objections to updated design taking into account the
> > comments
> > > I provided?
> > >
> >
> > Anton, I do not like an additional validator. I think you can accomplish
> > the same with a transaction event. You just need to design it more
> cleanly,
> > incorporating the feedback from Yakov.
> >
>

[Anton Vinogradov-2]

Dmitriy,
I fixed design according to your and Yakov's comments, thanks again for
clear explanation.

>> 1. You use internal API in public event, i.e. you cannot have user
>> accessing to IgniteInternalTx instance through TxEvent.

Event definition changed to
public class TransactionStartedEvent extends EventAdapter {
    private IgniteTransactions tx;
...
}

Not it's 100% public.

>> 2. Throwing runtime errors from listener is not documented and I doubt
if
>> it can be fully supported in the pattern you use in TxLabelTest. After
>> looking at the mentioned test user may think that throwing runtime error
>> when notified on new node join may prohibit new node joining which is
not
>> true. Do you have any example in Ignite when throwing exception from
>> listener is valid and documented.

Test's logic changed to
...
// Label
IgniteTransactions tx = evt.tx();

if (tx.label() == null)
tx.tx().rollback();
...
and
...
// Timeout
Transaction tx = evt.tx().tx();

if (tx.timeout() < 200)
tx.rollback();
...

So, tx will be rollbacked on creation and any commit attempt will cause
TransactionRollbackException

Full code listing available at
https://github.com/apache/ignite/pull/4036/files

Dmitriy, Yakov,
Could you please check and confirm changes?

чт, 24 мая 2018 г. в 16:32, Dmitriy Setrakyan <[hidden email]>:

> Anton, why do you need to *alter* event sub-system to introduce a new
> event? Yakov's issue was that you propagated private interface to public
> API, which is bad of course. Come up with a clean design and it will be
> accepted.
>
> My problem with TransactionValidator is that it only solves a small problem
> for transactions. If we do that, then we will have to add cache validators,
> compute validators, etc, etc, etc. That is why we either should use the
> existing event subsystem or come up with a holistic design that will work
> across the whole project.
>
> D.
>
> On Thu, May 24, 2018 at 1:38 AM, Anton Vinogradov <[hidden email]> wrote:
>
> > Dmitriy,
> >
> > Yakov is against the solution based on event sub-system
> > >> I think that we should think about some other solution instead of
> > altering
> > >> event sub-system.
> >
> > Also, I checked is there any chances to fix all the issues found by Yakov
> > and see that solution becomes overcomplicated in that case.
> > That's why I'm proposing this lightweight solution.
> >
> > As for me it's a good idea to have such validator since that's a common
> > problem at huge deployments when more than one team have access to Ignite
> > cluster and there is no other way to setup tx cretion rules.
> >
> > Yakov,
> >
> > Could you please share your thoughts on that?
> >
> >
> > чт, 24 мая 2018 г. в 8:58, Dmitriy Setrakyan <[hidden email]>:
> >
> > > On Wed, May 23, 2018 at 4:08 AM, Anton Vinogradov <[hidden email]>
> wrote:
> > >
> > > > Dmitriy, Yakov
> > > >
> > > > Are there any objections to updated design taking into account the
> > > comments
> > > > I provided?
> > > >
> > >
> > > Anton, I do not like an additional validator. I think you can
> accomplish
> > > the same with a transaction event. You just need to design it more
> > cleanly,
> > > incorporating the feedback from Yakov.
> > >
> >
>

[dsetrakyan]

Anton,

We cannot have TransactionStartedEvent without having events for all other
transaction states, like TransactionPreparedEvent,
TransactionCommittedEvent, etc. Considering this, I sill do not like the
design, as we would have to create many extra event classes.

Instead, I would suggest that you create TransactionStateChangeEvent, which
would have previous and new transaction state and would cover all state
changes, not just the start of the transaction. This will make the design
consistent and thorough.

D.

On Tue, May 29, 2018 at 5:39 AM, Anton Vinogradov <[hidden email]> wrote:

> Dmitriy,
> I fixed design according to your and Yakov's comments, thanks again for
> clear explanation.
>
> >> 1. You use internal API in public event, i.e. you cannot have user
> >> accessing to IgniteInternalTx instance through TxEvent.
>
> Event definition changed to
> public class TransactionStartedEvent extends EventAdapter {
>     private IgniteTransactions tx;
> ...
> }
>
> Not it's 100% public.
>
> >> 2. Throwing runtime errors from listener is not documented and I doubt
> if
> >> it can be fully supported in the pattern you use in TxLabelTest. After
> >> looking at the mentioned test user may think that throwing runtime error
> >> when notified on new node join may prohibit new node joining which is
> not
> >> true. Do you have any example in Ignite when throwing exception from
> >> listener is valid and documented.
>
> Test's logic changed to
> ...
> // Label
> IgniteTransactions tx = evt.tx();
>
> if (tx.label() == null)
> tx.tx().rollback();
> ...
> and
> ...
> // Timeout
> Transaction tx = evt.tx().tx();
>
> if (tx.timeout() < 200)
> tx.rollback();
> ...
>
> So, tx will be rollbacked on creation and any commit attempt will cause
> TransactionRollbackException
>
> Full code listing available at
> https://github.com/apache/ignite/pull/4036/files
>
> Dmitriy, Yakov,
> Could you please check and confirm changes?
>
> чт, 24 мая 2018 г. в 16:32, Dmitriy Setrakyan <[hidden email]>:
>
> > Anton, why do you need to *alter* event sub-system to introduce a new
> > event? Yakov's issue was that you propagated private interface to public
> > API, which is bad of course. Come up with a clean design and it will be
> > accepted.
> >
> > My problem with TransactionValidator is that it only solves a small
> problem
> > for transactions. If we do that, then we will have to add cache
> validators,
> > compute validators, etc, etc, etc. That is why we either should use the
> > existing event subsystem or come up with a holistic design that will work
> > across the whole project.
> >
> > D.
> >
> > On Thu, May 24, 2018 at 1:38 AM, Anton Vinogradov <[hidden email]> wrote:
> >
> > > Dmitriy,
> > >
> > > Yakov is against the solution based on event sub-system
> > > >> I think that we should think about some other solution instead of
> > > altering
> > > >> event sub-system.
> > >
> > > Also, I checked is there any chances to fix all the issues found by
> Yakov
> > > and see that solution becomes overcomplicated in that case.
> > > That's why I'm proposing this lightweight solution.
> > >
> > > As for me it's a good idea to have such validator since that's a common
> > > problem at huge deployments when more than one team have access to
> Ignite
> > > cluster and there is no other way to setup tx cretion rules.
> > >
> > > Yakov,
> > >
> > > Could you please share your thoughts on that?
> > >
> > >
> > > чт, 24 мая 2018 г. в 8:58, Dmitriy Setrakyan <[hidden email]>:
> > >
> > > > On Wed, May 23, 2018 at 4:08 AM, Anton Vinogradov <[hidden email]>
> > wrote:
> > > >
> > > > > Dmitriy, Yakov
> > > > >
> > > > > Are there any objections to updated design taking into account the
> > > > comments
> > > > > I provided?
> > > > >
> > > >
> > > > Anton, I do not like an additional validator. I think you can
> > accomplish
> > > > the same with a transaction event. You just need to design it more
> > > cleanly,
> > > > incorporating the feedback from Yakov.
> > > >
> > >
> >
>