[SPARK-5345][DEPLOY] Fix unstable test case in FsHistoryProviderSuite

[sarutak]

In FsHistoryProviderSuite, a test "Parse new and old application logs" sometimes fail and sometimes succeed. It's unstable.

[SparkQA]

Test build #25880 has finished for PR 4133 at commit 77678fe.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

Spark.testing should already be set for all tests. Is that needed ? What is the problem briefly and what does this fix?

[vanzin]

Can you explain how this fixes the problem? You're changing the order of the checks, but that doesn't match what the code does. For example, run this in a Scala shell:

scala> Seq((3L, 2L), (2L, 1L), (-1L, 2L), (-1L, 1L)).sortBy(x => (-x._1, -x._2))
res0: Seq[(Long, Long)] = List((3,2), (2,1), (-1,2), (-1,1))

That emulates what checkForLogs does in the sortBy call (sort first by end time descending, then by start time descending). As you can see, the entry with start time 2 should come before the entry with start time 1.

[vanzin]

In fact, your test change uncovered a bug. In FsHistoryProvider.scala, L214:

          if (newIterator.head.endTime > oldIterator.head.endTime) {

This will not merge the lists correctly for apps that have not finished yet. It should do a check similar to the sortBy clause above. Doesn't explain the original flakiness, though.

[andrewor14]

I haven't investigated the test in detail, but could the flakiness have been caused by the check interval? IIRC the history server doesn't actually start checking for logs until one full check interval has elapsed. If we assert before that happens, then obviously we're not gonna find the logs we expect. Could that be the problem?

[vanzin]

The flakiness might be because the polling timer is running in test mode, while the tests expect it not to run. @sarutak's patch seems to solve that, but that exposes a different bug that I mentioned above. I'll take a quick look at the code to see if that theory holds.

[vanzin]

Hmm... as Sean mentioned, this should already be defined here. Can you double check that it's really not set, and if not, what's causing it?

[vanzin]

Hi @sarutak ,

Check out https://github.com/vanzin/spark/tree/SPARK-5345. That fixes the sort problem, and cleans up the code a bit. It doesn't explain how the thread might be started during testing, though. But it doesn't seem to be happening with this patch (I even added an exception - not in the patch - to check that's the case, which wasn't triggered).

[vanzin]

I hit the exception I added for testing when running all tests, but never when running the test in isolation. It looks like some test is clearing the system properties (or that particular one), which is bad for other reasons, and I don't think should be worked around here.

[vanzin]

BTW it's very likely this is caused by the issue described in #4220 (comment).

[JoshRosen]

Jenkins, retest this please.

[JoshRosen]

I think we've observed this test's flakiness even after fixing #4220, so we should continue investigating it.

[SparkQA]

Test build #26640 has finished for PR 4133 at commit 77678fe.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[vanzin]

Even if the flakiness hasn't been fixed yet, this patch is still not correct. The test itself is correct, the FsHistoryProvider implementation has a bug; see patch I linked to above (https://github.com/vanzin/spark/tree/SPARK-5345).

[srowen]

Shall we close this PR in favor of #4370 ?

[vanzin]

This PR cannot be pushed as is, since it makes the test test the buggy sort behavior instead of correcting it. Still, if after #4220 these tests still fail, there's something still to be done for the bug.

[JoshRosen]

I think that we should close this for the time being, since we can always re-open if we see the bug again.

[sarutak]

O.K, I close this PR for now.