Yes, currently handler handles exception in the same way as
'GridAbstractTest', so, it will work from any thread.
пт, 11 янв. 2019 г., 15:18 Ilya Lantukh ilant...@gridgain.com:
> Dmitry,
>
> It doesn't make sense to run that test now because the root cause of it's
> failure had been fixed.
>
>
Dmitry,
It doesn't make sense to run that test now because the root cause of it's
failure had been fixed.
You should verify that getting an unhandled exception in any system thread
leads to the failure of currently running test.
On Fri, Jan 11, 2019 at 12:16 PM Dmitrii Ryabov
wrote:
> Ilya,
Ilya, can you check your test on current implementation [1]?
[1] https://github.com/apache/ignite/pull/5662
10 дек. 2018 г. 17:10 пользователь "Dmitriy Pavlov"
написал:
Reverted.
https://issues.apache.org/jira/browse/IGNITE-8227 reopened
пн, 10 дек. 2018 г. в 16:23, Dmitriy Pavlov :
>
Reverted.
https://issues.apache.org/jira/browse/IGNITE-8227 reopened
пн, 10 дек. 2018 г. в 16:23, Dmitriy Pavlov :
> Anton, I was expecting that you revert, because you wanted to do it.
>
> Provided that I agree that fix could be reverted because of both
> functional and style possible
Anton, I was expecting that you revert, because you wanted to do it.
Provided that I agree that fix could be reverted because of both functional
and style possible improvements, does not mean I believe it is the only
option and it should be reverted.
Even if I agree to revert doesn't mean all
Dmitriy,
You confirmed that fix should be reverted and reworked last Friday.
Why it still not reverted?
On Mon, Dec 10, 2018 at 12:46 AM Dmitrii Ryabov
wrote:
> Agree, it is reasonable to revert.
> пт, 7 дек. 2018 г. в 18:44, Dmitriy Pavlov :
> >
> > Hi Ilya,
> >
> > thank you for noticing.
>
Agree, it is reasonable to revert.
пт, 7 дек. 2018 г. в 18:44, Dmitriy Pavlov :
>
> Hi Ilya,
>
> thank you for noticing.
>
> Calling to fail is equal to re-throw,
>
> throw new AssertionFailedError(message);
>
> So, yes, for now it is absolutely valid reason to revert and rework fix
>
> -
Hi Ilya,
thank you for noticing.
Calling to fail is equal to re-throw,
throw new AssertionFailedError(message);
So, yes, for now it is absolutely valid reason to revert and rework fix
- as Nikolay suggested to reduce method override ocurrences.
- and with transferring this exception
Unfortunately, this FailureHandler doesn't seem to work. I wrote a test
that reproduces a bug and should fail. It prints the following text into
log, but the test still passes "successfully":
[2018-12-07
>> We stop, for now, then you will chill a
>> little bit, then you will have an absolutely fantastic weekend, and then
on
>> Monday, Dec 10 we will continue this discussion in a positive and
>> constructive manner.
Agree
On Thu, Dec 6, 2018 at 3:55 PM Nikolay Izhikov wrote:
> Anton.
>
> I
Anton.
I discussed this fix privately with Dmitriy Pavlov.
1. We had NoOpHandler for ALL tests before this merge.
2. Dmitry Ryabov will remove all copypasted code soon.
So, this fix make things better.
I think we shouldn't revert it.
I think we should continue work to turn off NoOpHandler in
Anton, I have another proposal. We stop, for now, then you will chill a
little bit, then you will have an absolutely fantastic weekend, and then on
Monday, Dec 10 we will continue this discussion in a positive and
constructive manner.
Trying to win in a match "my revert is bigger than yours/my
>> I still hope Anton will do the first bunch of tests research to
demonstrate
>> the idea.
Dmitriy,
Just want to remind you that we already spend time here because of
unacceptable code merge situation.
Such merges should NEVER happen again.
Please, next time make sure that code you merge has no
Anton, I mean `copy-paste reduce` ticket. I'll try to describe reasons for
no-op in tests. Then, we can create tickets to fix this cases if needed.
чт, 6 дек. 2018 г., 13:53 Dmitriy Pavlov dpav...@apache.org:
> BTW, No-Op or StopNode-FailTest in case of a deep investigation will always
> require
Nikolay,
Answering your questions a couple of emails ago.
Only one valid reason to avoid NoOp it the risk
- we don't correctly understand test meaning by class name, we don't catch
it's expected flow and
- there is some test which uses NoOp now, but should not.
Any failure in such test included
BTW, No-Op or StopNode-FailTest in case of a deep investigation will always
require to understand what test does and what it tests.
So we can get a positive outcome from this research if we agree to add
- a small description to each test about the reason for existing of this
test,
- what is the
Dmitrii,
>> I agree with Nikolay's solution. If no one minds, I'll create ticket for
>> appropriate changes and recheck issues.
Do you mean 'copy-paste reduce' ticket or check/fix of all tests with no-op
to have a proper handler?
Just want to make sure that copy-paste minimization is not the
Dmitrii Ryabov,
Your comments sounds reasonable to me. Marker base class approach
looks good to me so far.
P.S. I had even worse name in mind 'StopGaps' =)
чт, 6 дек. 2018 г. в 13:08, Dmitrii Ryabov :
>
> Ivan, I think `Workarounds` class isn't good idea, because it looks like we
> create stable
Ivan, I think `Workarounds` class isn't good idea, because it looks like we
create stable workarounds, which will never be fixed.
I agree with Nikolay's solution. If no one minds, I'll create ticket for
appropriate changes and recheck issues.
чт, 6 дек. 2018 г., 12:17 Anton Vinogradov
Folks, thank's everyone for solution research.
I'm ok with Nikolay approach in case that's not a final step.
On Thu, Dec 6, 2018 at 12:11 PM Павлухин Иван wrote:
> Nikolay,
>
> I meant "not expensive" by "cheap". And I meant that it is good that
> it cheap =). And I said it to contrast with
Ivan,
Got it.
Thanks for the explanation.
> mostly I would like an opinion from Dmitriy Ryabov as an original author
Dmitriy, can you answer?
I can do this improvement by myself, by if you want to do it - go ahead.
чт, 6 дек. 2018 г. в 12:11, Павлухин Иван :
> Nikolay,
>
> I meant "not
Nikolay,
I meant "not expensive" by "cheap". And I meant that it is good that
it cheap =). And I said it to contrast with "expensive" ~100 tests
investigation. And if we agree (mostly I would like an opinion from
Dmitriy Ryabov as an original author) on a way how to improve the
patch then let's
Dmitriy Ryabov, Dmitriy Pavlov, sorry.
Of course it should be "NOT to blame author".
Sorry, one more time.
чт, 6 дек. 2018 г., 10:40 Dmitriy Pavlov dpav...@apache.org:
> I hope you've misprinted here
> > I'm here to blame the author.
>
> We can blame code but never coders.
>
> Please see
I hope you've misprinted here
> I'm here to blame the author.
We can blame code but never coders.
Please see https://discourse.pi-hole.net/faq - has absolutely nothing in
common with Apache Guides, but says the same things. It is a practical
necessity to maintain a friendly atmosphere.
чт, 6
Ivan.
> 1. Accept the patch and bring an improvement to Ignite (and create a> ticket
> for further investigation).
I support this idea.
Do we create the tickets already?
> Nikolay's patch [1] suggests a slightly different approach how to the
> same thing. And implementing that idea looks like
Guys,
I asked what harm will applying the patch bring I have not got a
direct answer. But I think I got some pain points:
1. Anton does not like that reasons why ~100 tests require noop
handler are not clear. And might be several problems are covered
there.
2. Nikolay suggests some code
Dmitriy.
> The closest analog to Noop handler is mute of test failure.
> By this commit, we had unmuted (possible) failures in ~5-~100=~49900
tests, and we’re still concerned about style or minor details if no-op was
copy-pasted, aren’t we?
Can you explain this idea a bit more?
I don't
> Thanks, as an improvement to the code, this may be better.
I can prepare a full patch for NoOp handler.
What do you think?
Anton Vinogradov, do you agree with this approach?
ср, 5 дек. 2018 г. в 20:33, Dmitriy Pavlov :
> Thanks, as an improvement to the code, this may be better. But still,
Thanks, as an improvement to the code, this may be better. But still, it is
not a reason to revert. And Anton mentioned something with better exception
handling/logging. Probably we will see an implementation as well.
This case here is a big thing related to The Apache Way, - and I'll explain
why
Dmitriy.
I think we should avoid copy paste code instead of thinking about Apache
Way all the time :)
Anyway, I propose to return to the code!
I think we should use some kind of marker base class for a cases with
NoOpHandler.
This has several advantages, comparing with current implementation:
Folks, let me explain one thing which is not related much to fix itself,
but it is more about how we interact. If someone will just come to the list
and say it is not good commit, it is a silly solution and say to others to
rework these patches - it is a road to nowhere.
If someone sees the
As I can see from the above discussion,
> Tests in these classes check fail cases when we expect critical failure
like node stop or exception thrown
So, this copy-n-paste-style change is caused by the imperfect logic of
existing tests, that should be reworked in more robust way, e.g. using
Hello, Igniters.
I'm agree with Anton Vinogradov.
I think we should avoid commits like [1]
Copy paste coding style is well known anti pattern.
Don't we have another option to do same fix with better styling?
Accepting such patches leads to the further tickets to cleanup mess that
patches
Andrey,
>> But why should we make all things perfect
>> in a single fix?
As I said, I'm ok in case someone ready to continue :)
But, we should avoid such over-copy-pasted commits in the future.
On Wed, Dec 5, 2018 at 5:13 PM Andrey Mashenkov
wrote:
> Dmitry,
>
> Do we have TC run results for
Dmitry,
Do we have TC run results for the PR before massive failure handler
fallbacks were added?
Let's create a ticket to investigate possibility of using any meaningful
failure handler for such tests with TC report attached.
On Wed, Dec 5, 2018 at 4:41 PM Anton Vinogradov wrote:
> Dmitriy,
>
Anton,
I really like your perfectionism. But why should we make all things perfect
in a single fix? The change you want to roll back is definitely useful for
the project: the majority of our tests do not hide potential bugs under
no-op handler anymore, and the small number of tests require
Dmitriy,
It's ok in case someone ready to do this (get rid of all no-op or explain
why it's a better choice).
Explicit confirmation required.
Otherwise, only rollback is an option.
On Wed, Dec 5, 2018 at 4:29 PM Dmitriy Pavlov wrote:
> Anton, if you care enough here will you try to research a
Anton, if you care enough here will you try to research a couple of these
tests? Or you are asking others to do things for you, aren't you?
I like idea from Andrew to create ticket and check these test to keep
moving towards 010 tests with noop. It is easy to locate these
overridden method
>> I didn't get. What is the problem in saving No-Op for several tests? Why
>> should we keep No-Op for all?
Several (less than 10) is ok to me with the proper explanation why tests
fail and why no-op is a better choice.
100+++ copy-pasted no-op handlers are not ok!
>> I don't ask you to re-do
Guys,
I didn't get. What is the problem in saving No-Op for several tests? Why
should we keep No-Op for all?
On Wed, Dec 5, 2018 at 3:20 PM Павлухин Иван wrote:
> Anton,
>
> Yes I meant that patch. And I would like to respell a name "massive
> no-op handler restore" to "use no-op failure
Anton, I disagree with this approach: "You will ask, other will provide
explanations/excuses/apology and so on". Since you rejecting to chime in
and help this means trying to manage instead of doing.
I don't ask you to re-do this change, I ask to demonstrate any better
approach for tests which
Ivan,
>> Yes I meant that patch. And I would like to respell a name "massive
>> no-op handler restore" to "use no-op failure handler only where it is
>> assumed".
How about "we changed some handlers to proper, but keep other no-ops using
explicit copy-paste"? :)
On Wed, Dec 5, 2018 at 3:38 PM
Dmitriy Pavlov, Dmitrii Ryabov,
>> Anton, there is no reason to revert other's contributions because you
know
>> how to do things better.
What I see is "We replaced no-op with the proper handler, but . 100+
no-op still here because tests start failing :)"
That's a completely different
Anton,
Yes I meant that patch. And I would like to respell a name "massive
no-op handler restore" to "use no-op failure handler only where it is
assumed".
ср, 5 дек. 2018 г. в 15:09, Dmitriy Pavlov :
>
> Dmitrii Ryabov explained these tests are perfectly ok to have failures as
> these tests do
Dmitrii Ryabov explained these tests are perfectly ok to have failures as
these tests do test failures.
Anton, there is no reason to revert other's contributions because you know
how to do things better. A lot of people can do things better than me.
Should we revert everything I've contributed? I
Ivan,
Do you mean massive no-op handler restore patch [1]?
[1] https://github.com/apache/ignite/pull/4974/files
On Wed, Dec 5, 2018 at 2:53 PM Павлухин Иван wrote:
> Hi Anton,
>
> Could you please summarize what does aforementioned patch made really
> worse?
>
> As I see, the patch added a
Hi Anton,
Could you please summarize what does aforementioned patch made really worse?
As I see, the patch added a very good thing -- meaningful failure
handler in tests. And I think it is really important. But was is the
harm and does it overweight positive result? And why?
ср, 5 дек. 2018 г. в
Dmitriy,
That's an incorrect idea to ask me to provide PR or to fix these test
properly since I'm not an author or reviewer.
But, I, as a community member, ask you to explain what problems the fix
fixes.
In case you're not able to provide the explanation I will rollback the
changes.
That's not
Anton, please provide PR to demo your idea. Code speaks louder than words
sometimes.
No reason to revert a contribution if someone has an idea, which is not
clear for others.
Again, we should discuss not Dmitrii contribution, but the initial
selection of no-op.
If you will do a test failure
Dmitriy,
As I said before, these changes allow tests to be successful in case of
unexpected failures.
That's not acceptable.
As a reviewer, you have to be ready to provide arguments why these tests
have to be fixed this way and what was the problem, in case you merged such
changes.
That's
I will not do any rollback because changes make tests better. Please pay
attention that no-op became default long time ago. Please discuss this
selection with authors of the previous commit. New commit changes
NoOp->FailTest+stopNode.
Please provide a PR to demonstrate your idea how to transfer
Dmitriy,
>> Which code block will do a throw?
Depends on the test.
Looks like we make the *bad *test even *worse*.
That's not a correct fix.
In case you expect failure you have to check this expectation inside the
special handler.
I'd like to ask you to rollback these changes and replace them
Hi,
Dmitri,
The meaningful failure handler as a default one looks reasonable.
Thanks a lot.
But what is the reason to fallback to noop for 100+ test?
Does it means these test become failed after changing default failure
handler?
If so, let's create a ticket (may be umbrella) to investigate and
Anton,
If I understood this idea right, try-catch will not work because failure
can be thrown into an Ignite thread pool, which catches any exceptions and
errors.
Which code block will do a throw?
Sincerely,
Dmitriy Pavlov
ср, 5 дек. 2018 г. в 12:16, Anton Vinogradov :
> Dmitrii,
>
> No-op
Dmitrii,
No-op means "hide any problem", so, we lose the guarantees.
Could you please share some examples where "no-op" better than "strict
try-catch with a check"?
On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov
wrote:
> Anton, I think wrapping every disconnecting node with try-catch will be
>
Anton, I think wrapping every disconnecting node with try-catch will be
less readable than no-op handler.
ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov dpav...@apache.org:
> Folks let me remind you that Dmitry changed default of ALL tests from noop
> to a meaningful handler. So we should start every
Folks let me remind you that Dmitry changed default of ALL tests from noop
to a meaningful handler. So we should start every message here from saying
thank you to Dmitry.
Please review remaining tests and remove noop where possible.
вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov :
> Hi all,
>
>
Hi all,
Really, why noop?
If you expect failure handler should be triggered, you can override default
one and rise some flag, which can be checked in test.
This will make test clearer.
With noop, you'll get previous unwanted behavior, that you are trying to
improve, isnt'it?
4 дек. 2018 г.
And you have to check the reason of failure inside the try-catch block, of
course.
In case found not equals to expected then test should rethrow the exception.
вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov :
> Dmitrii,
>
> The solution is not clear to me.
> In case you expect the failure then a
Dmitrii,
The solution is not clear to me.
In case you expect the failure then a correct case is to wrap it with
try-catch block instead of no-op failure handler usage.
вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov :
> Anton,
>
> Tests in these classes check fail cases when we expect critical
>
Anton,
Tests in these classes check fail cases when we expect critical
failure like node stop or exception thrown. Such tests trigger failure
handler and it fails test when everything goes as it should go. That's
why we need no-op handler here.
вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov :
>
> Hi
Hi Igniters,
BTW, if you find in any of your tests it does't need an old value of
handler (=NoOp), feel free to remove it.
Sincerely,
Dmitriy Pavlov
вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov :
> Dmitrii,
>
> Could you please explain the reason of explicit set of 100+
> NoOpFailureHandlers?
Dmitrii,
Could you please explain the reason of explicit set of 100+
NoOpFailureHandlers?
вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov :
> Hello, Igniters!
>
> Today the test framework's default no-op failure handler was changed to the
> handler, which stops the node and fails the test.
>
> Over
Hello, Igniters!
Today the test framework's default no-op failure handler was changed to the
handler, which stops the node and fails the test.
Over 100 tests kept no-op failure handler by overrided
`getFailureHandler()` method.
If you'll found a problem or something unexpected - write here or
64 matches
Mail list logo
