[SPARK-28931][CORE][TESTS] Fix couple of bugs in FsHistoryProviderSuite#25629
[SPARK-28931][CORE][TESTS] Fix couple of bugs in FsHistoryProviderSuite#25629HeartSaVioR wants to merge 2 commits intoapache:masterfrom
Conversation
|
I've found the bug while working on SPARK-28869. While it doesn't make test failures in master branch, it triggers failures on my work so just propose the fix earlier. |
|
Test build #109939 has finished for PR 25629 at commit
|
|
Nice, @HeartSaVioR .
|
|
I'll review after that. |
|
Also, cc @vanzin |
dongjoon-hyun
changed the title
dongjoon-hyun
changed the title
HeartSaVioR
changed the title
|
Thanks @dongjoon-hyun , I just filed the JIRA issue and modified the title. |
| @@ -1256,12 +1257,7 @@ class FsHistoryProviderSuite extends SparkFunSuite with Matchers with Logging { | |||
| val cstream = codec.map(_.compressedOutputStream(fstream)).getOrElse(fstream) | |||
| val bstream = new BufferedOutputStream(cstream) | |||
| if (isNewFormat) { | |||
There was a problem hiding this comment.
We don't support the old log format anymore, so you could actually remove this parameter since you're here...
|
Test build #109965 has finished for PR 25629 at commit
|
|
Ur, it's relevant failure. Although I'll trigger this once more, could you take a look?
|
|
Retest this please. |
|
It seems to be known flaky - https://issues.apache.org/jira/browse/SPARK-28770 - and not related to this patch |
|
Test build #109977 has finished for PR 25629 at commit
|
|
Test failure for 109977 is |
|
retest this, please |
|
Test build #109981 has finished for PR 25629 at commit
|
|
cc. @felixcheung as this patch is a part of work in SPARK-28869 (found this during working) |
|
Merging to master. |
|
Thanks all for reviewing and merging! |
This patch fixes the bugs in test code itself, FsHistoryProviderSuite. 1. When creating log file via `newLogFile`, codec is ignored, leading to wrong file name. (No one tends to create test for test code, as well as the bug doesn't affect existing tests indeed, so not easy to catch.) 2. When writing events to log file via `writeFile`, metadata (in case of new format) gets written to file regardless of its codec, and the content is overwritten by another stream, hence no information for Spark version is available. It affects existing test, hence we have wrong expected value to workaround the bug. This patch also removes redundant parameter `isNewFormat` in `writeFile`, as according to review comment, Spark no longer supports old format. Explained in above section why they're bugs, though they only reside in test-code. (Please note that the bug didn't come from non-test side of code.) No Modified existing UTs, as well as read event log file in console to see metadata is not overwritten by other contents. Closes apache#25629 from HeartSaVioR/MINOR-FIX-FsHistoryProviderSuite. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan@gmail.com> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
5 participants
What changes were proposed in this pull request?
This patch fixes the bugs in test code itself, FsHistoryProviderSuite.
newLogFile, codec is ignored, leading to wrong file name. (No one tends to create test for test code, as well as the bug doesn't affect existing tests indeed, so not easy to catch.)writeFile, metadata (in case of new format) gets written to file regardless of its codec, and the content is overwritten by another stream, hence no information for Spark version is available. It affects existing test, hence we have wrong expected value to workaround the bug.This patch also removes redundant parameter
isNewFormatinwriteFile, as according to review comment, Spark no longer supports old format.Why are the changes needed?
Explained in above section why they're bugs, though they only reside in test-code. (Please note that the bug didn't come from non-test side of code.)
Does this PR introduce any user-facing change?
No
How was this patch tested?
Modified existing UTs, as well as read event log file in console to see metadata is not overwritten by other contents.