Fix ForkingTaskRunnerTest

[kfaraz]

ForkingTaskRunner uses static fields to track task counts which are reported in metrics.
This causes some tests like ForkingTaskRunnerTest.testInvalidTaskContextJavaOptsArray() to give false positives if the variables had been set by a preceding test.

Changes:

• Use non-static fields to track task counts in ForkingTaskRunner
• Update assertions in tests to ensure that the tests are idempotent.

[IgorBerman]

@kfaraz don't you think there should be
failedTaskCount .incrementAndGet();
near LOGGER.info(t, "Exception caught during execution"); at wrapping try-catch?
I mean when we pass bad arguments the IllegalArgumentException is thrown but the code never increments failedTaskCount

[IgorBerman]

btw, those counters used in metrics reporting, so removing static will cause change in those metrics... i mean from test perspective static causing races, but if we looking at metric level and WorkerTaskCountStatsProvider I can understand why it's static since it's "Proides task / task count status at the level of individual worker nodes"

[IgorBerman]

if testing framework makes sure that each test runs one after another and not in parallel, other option would be to reset static variables in some visible for test method.
Additional option would be to use some interface that a) decouples from ForkingTaskRunnerTest & WorkerTaskCountStatsProvider and b) "collects" reports from ForkingTaskRunnerTest. for tests it will 1 implementation, race free.
for production it can wrap those static variables and WorkerTaskCountStatsProvider will take those reports from this interface

[kfaraz]

but if we looking at metric level and WorkerTaskCountStatsProvider I can understand why it's static since it's "Proides task / task count status at the level of individual worker nodes"

@IgorBerman , I am not sure I follow. How would this be affected if we make the fields non-static?
ForkingTaskRunner is used only by MiddleManagers and each MiddleManager would have only a single instance of it. Keeping the fields static doesn't serve any purpose, afaict.

[kfaraz]

don't you think there should be
failedTaskCount .incrementAndGet();
near LOGGER.info(t, "Exception caught during execution"); at wrapping try-catch?

Yes, I wanted to do this but I decided to do it later as I need to look at all the exceptions and if it would be better to not throw an exception at all and simply return a TaskStatus.failure() in those cases. But I need to test it properly before making those changes.

The changes in this PR are more straightforward and just meant to get the build right without altering any behaviour.

[IgorBerman]

oh, ok, if it's singleton then it will work and no need in static. I missed this part

[AmatyaAvadhanula]

Why are both failed and successful counts 0?

[kfaraz]

Yes, ideally the failed count should be 1, but it is currently not being tracked.
We can fix up the code later as mentioned here: #16323 (comment).

[AmatyaAvadhanula]

Thanks for the clarification, @kfaraz. LGTM

[kfaraz]

Thanks for the review, @AmatyaAvadhanula , @IgorBerman !