-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-5600] [core] Clean up FsHistoryProvider test, fix app sort order. #4370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Clean up some test setup code to remove duplicate instantiation of the provider. Also make sure unfinished apps are sorted correctly.
|
Test build #26768 has started for PR 4370 at commit
|
|
Test build #26768 has finished for PR 4370 at commit
|
|
Test FAILed. |
|
Jenkins, retest this please. |
|
Test build #26779 has started for PR 4370 at commit
|
|
Test build #26779 has finished for PR 4370 at commit
|
|
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest LGTM, but, isn't this defining a different and inconsistent sort order? For example if the first argument's start and end time are after and before the other's start and times, respectively, then the method returns false both ways, when one has to be true. The existing logic is to sort by endTime descending, then startTime descending, so I think it's more like...
if (i1.endTime != i2.endTime) {
i1.endTime > i2.endTime
} else {
i1.startTime > i2.startTime
}
Rename variables / apps so that it's easier to follow which one is which, and also tweak the app times so that they better test the sorting code.
|
Test build #26939 has started for PR 4370 at commit
|
|
Test build #26939 has finished for PR 4370 at commit
|
|
Test PASSed. |
|
LGTM I'm merging this into master 1.3 thanks @vanzin |
Clean up some test setup code to remove duplicate instantiation of the provider. Also make sure unfinished apps are sorted correctly. Author: Marcelo Vanzin <vanzin@cloudera.com> Closes #4370 from vanzin/SPARK-5600 and squashes the following commits: 0d048d5 [Marcelo Vanzin] Cleanup test code a bit. 2585119 [Marcelo Vanzin] Review feedback. 8b97544 [Marcelo Vanzin] Merge branch 'master' into SPARK-5600 be979e9 [Marcelo Vanzin] Merge branch 'master' into SPARK-5600 298371c [Marcelo Vanzin] [SPARK-5600] [core] Clean up FsHistoryProvider test, fix app sort order. (cherry picked from commit 5687bab) Signed-off-by: Andrew Or <andrew@databricks.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't imagine it probably matters, but this should be > instead of >= I think? or else this says that two equal intervals should both precede the other. I bet it doesn't make the sort wrong, just might affect whether the sort is stable, but who cares here.
Clean up some test setup code to remove duplicate instantiation of the
provider. Also make sure unfinished apps are sorted correctly.