Skip to content

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Apr 17, 2015

Title says it all.

cc @rxin @tdas

@srowen
Copy link
Member

srowen commented Apr 17, 2015

If you have a moment, this is more for my edification, is the benefit simply that there is no need to participate in the full-blown actor system, and that this is simpler to reason about as a local event loop? are there other benefits or drawabacks?

@SparkQA
Copy link

SparkQA commented Apr 17, 2015

Test build #30480 has finished for PR 5554 at commit ec6ec58.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 17, 2015

@srowen this is a part of SPARK-5293. The future plan is make Spark be able to be built without Akka at all eventually.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 17, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Apr 17, 2015

Test build #30479 has finished for PR 5554 at commit ce0fa73.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@zsxwing zsxwing changed the title [SPARK-6979][Streaming] Replace JobScheduler.eventActor with EventLoop [SPARK-6979][Streaming] Replace JobScheduler.eventActor and JobGenerator.eventActor with EventLoop Apr 17, 2015
@SparkQA
Copy link

SparkQA commented Apr 17, 2015

Test build #30482 has finished for PR 5554 at commit ec6ec58.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@SparkQA
Copy link

SparkQA commented Apr 17, 2015

Test build #30485 has finished for PR 5554 at commit e496ace.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 17, 2015

I just replaced jobScheduler.reportError("Error in job generator", e) with logError("Error in job generator", e) and NotSerializableException disappeared. Not sure why. Will dig deeply tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a blank line here

@SparkQA
Copy link

SparkQA commented Apr 17, 2015

Test build #30486 has finished for PR 5554 at commit 633e279.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@rxin
Copy link
Contributor

rxin commented Apr 19, 2015

Thanks - please ping me when this is ready.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 19, 2015

Finally, figured out why logError("Error in job generator", e) won't fail the test cases.

NotSerializableException also happens in the old codes, but it just forces the Actor restart because it's not caught by any code. Then I use EventLoop to replace Actor, so NotSerializableException will be caught and sent to the onError method. If I use logError("Error in job generator", e), NotSerializableException is just logged and doesn't fail the tests. But if I use jobScheduler.reportError("Error in job generator", e), the error will be propagated to the mail thread of the tests. That's why the tests fail.

StreamingKMeans must be Serializable since it will be used in the closure.

So it's a bug that was hidden by Actor error mechanism. And now jobScheduler.reportError exposes the bug.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 19, 2015

Just added Serializable to StreamingKMeans. @rxin if you think I need to open a JIRA for the NotSerializableException bug in StreamingKMeans, please let me know.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 19, 2015

I think I should create a new PR for the NotSerializableException bug in StreamingKMeans so that we can merge it to branch 1.2 and 1.3. Added #5582 for the bug.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 19, 2015

Will update this PR once #5582 is merged.

@SparkQA
Copy link

SparkQA commented Apr 19, 2015

Test build #30565 has finished for PR 5554 at commit 5304350.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@rxin
Copy link
Contributor

rxin commented Apr 20, 2015

LGTM. I'm going to merge it in master since it doesn't conflict.

@asfgit asfgit closed this in c776ee8 Apr 20, 2015
@zsxwing zsxwing deleted the SPARK-6979 branch April 20, 2015 09:43
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
…tor.eventActor with EventLoop

Title says it all.

cc rxin tdas

Author: zsxwing <[email protected]>

Closes apache#5554 from zsxwing/SPARK-6979 and squashes the following commits:

5304350 [zsxwing] Fix NotSerializableException
e9d3479 [zsxwing] Add blank lines
633e279 [zsxwing] Fix NotSerializableException
e496ace [zsxwing] Replace JobGenerator.eventActor with EventLoop
ec6ec58 [zsxwing] Fix the import order
ce0fa73 [zsxwing] Replace JobScheduler.eventActor with EventLoop
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