JAVA-3117: Call CcmCustomRule#after if CcmCustomRule#before fails to …

[hhughes]

…allow subsequent tests to run

[dkropachev]

You forgot to do the same for after

[adutra]

Suggested change

	LOG.warn("Error cleaning up CustomCcmRule before() failure", e1);
	LOG.warn("Error cleaning up CustomCcmRule before() failure", e1);
	e.addSuppressed(e1);

[dkropachev]

@adutra , @hhughes , please don't merge it, you need to wrap super.after(); with try catch as well.

[lukasz-antoniak]

Wrapping after is not necessary, as follow-up before would clean things up.

[dkropachev]

Wrapping after is not necessary, as follow-up before would clean things up.

it super().after() fails, CURRENT stays populated and you endup with the exact same problem.

[tolbertam]

it super().after() fails, CURRENT stails populated and you endup with the exact same problem.

👍 I think I see what you mean (CURRENT.compareAndSet(this, null) not being executed in after, would cause the next invocation of before to continue throwing an IllegalStateException as CURRENT is not null)), and that seems easy enough to add. Given this is an old PR I suggest we merge this and create a separate PR that addresses both this and @adutra's suggestion and merge that.

[tolbertam]

👍 we should merge this and do a follow on to address the two comments.

[dkropachev]

@tolbertam , @hhughes @lukasz-antoniak, ok, let's merge it i will create a follow up PR

[dkropachev]

Another question, will it break RuleChain sequence ?
Meaning if CustomCcmRule is part of the RuleChain:
CustomCcmRule, Rule#1
then RuleChain.before calls CustomCcmRule.before and then Rule#1.before
when test or any rule fails it is going to call:
RuleChain.after which is going to call Rule#1.after and only then CustomCcmRule.after.
If you do call after() here it changes the sequence and can cause unexpected consuquences.

[dkropachev]

@tolbertam , @hhughes @lukasz-antoniak, ok, let's merge it i will create a follow up PR

Or maybe I can create a new PR to address it.

[tolbertam]

@dkropachev sounds good; I'll merge this as is shortly to pick up @hhughes's contribution and if you could follow up with a PR with the suggested additions that would be great.

[tolbertam]

merged, I'll keep my eye out for your PR @dkropachev 👍

[lukasz-antoniak]

Wrapping after is not necessary, as follow-up before would clean things up.

it super().after() fails, CURRENT stays populated and you endup with the exact same problem.

It is a valid point. However, tests usually run ccm start in before phase and ccm remove in after phase. If ccm remove fails (or any other exception is thrown by super.after()) it would indicate a bug in CCM project, but then potentially running C* node may not have been terminated. Trying to ccm start another node could also cause problems and fail for subsequent executions (e.g. TCP port already in use). Based on experience running driver tests on Jenkins, I have never seen ccm remove fail.

👍 for attempting to handle failures of super.after() and rule chain sequence.

To justify merging this PR - it will still greatly reduce situation when single ccm start failure causes subsequent tests to fail. ccm start failures do not happen often. Currently, when this happens more tests get reported as failed on Jenkins and it is harder to point flaky tests. ccm start can indeed be a cause of test failures. A common example is test with old C* / DSE version in the backend that tries to use a feature introduced later.