Skip to content

Conversation

@erenavsarogullari
Copy link
Member

What changes were proposed in this pull request?

  • TaskState and ExecutorState expose isFailed and isFinished functions. It can be useful to add test coverage for different states. Currently, Other enums do not expose any functions so this PR aims just these two enums.
  • private access modifier is added for Finished Task States Set
  • A minor doc change is added.

How was this patch tested?

New Unit tests are added and run locally.

@rxin
Copy link
Contributor

rxin commented Sep 18, 2016

hm I agree having good unit test coverage is important -- this seems too trivial to test? It is also part of the code that doesn't really change.

@erenavsarogullari
Copy link
Member Author

Hi @rxin,

Firstly, thanks for quick reply.

I was thinking for unit-test coverage perspective and a starter point to contribute project but it is ok for me if PR is closed as well ;)

@srowen
Copy link
Member

srowen commented Sep 20, 2016

This is to some degree covered by other tests, yes. It's probably just not worth 100 lines of test code to test the contents of a Set from all angles.

I think the other two non-test changes are trivial but I'd be willing to commit them.

@erenavsarogullari
Copy link
Member Author

Thanks @srowen for the comment. The change-set looks ready.

@srowen
Copy link
Member

srowen commented Sep 21, 2016

OK, can you modify the title? it becomes the commit message. "Minor code change to TaskState and Task" or something.

Normally I'd say this is too trivial to commit by itself but I know it didn't start that way, and because it's a first commit I'm more inclined to merge even a little part of it to get you through the process once.

@erenavsarogullari erenavsarogullari changed the title [SPARK-17584][Test] - Add unit test coverage for TaskState and ExecutorState [CORE][MINOR] - Add minor code change to TaskState and Task Sep 21, 2016
@erenavsarogullari
Copy link
Member Author

Thanks @srowen. PR Title is updated.

@SparkQA
Copy link

SparkQA commented Sep 21, 2016

Test build #3282 has finished for PR 15143 at commit da2da9f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Sep 21, 2016

Merged to master

@asfgit asfgit closed this in dd7561d Sep 21, 2016
@erenavsarogullari erenavsarogullari deleted the SPARK-17584 branch September 24, 2016 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants