Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SPARK-5174][SPARK-5175] provide more APIs in ActorHelper #3984

Closed
wants to merge 3 commits into from
Closed

[SPARK-5174][SPARK-5175] provide more APIs in ActorHelper #3984

wants to merge 3 commits into from

Conversation

CodingCat
Copy link
Contributor

https://issues.apache.org/jira/browse/SPARK-5174

https://issues.apache.org/jira/browse/SPARK-5175

  1. add more APIs and scala docs in ActorHelper
  2. fix several bugs in the existing message handler in actor receiver supervisor

@CodingCat
Copy link
Contributor Author

Hi, @tdas, do you mind reviewing this?

@SparkQA
Copy link

SparkQA commented Jan 9, 2015

Test build #25342 has finished for PR 3984 at commit d5a57db.

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

sarutak
sarutak reviewed Jan 10, 2015
View reviewed changes
streaming/src/main/scala/org/apache/spark/streaming/receiver/ActorReceiver.scala

val n: AtomicInteger = new AtomicInteger(0)
val hiccups: AtomicInteger = new AtomicInteger(0)

Copy link
Member

Choose a reason for hiding this comment

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

Why do you stop using AtomicInteger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

supervisor is single-threaded , I don't think we have scenario where we update concurrently

Copy link
Member

Choose a reason for hiding this comment

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

I think, it's not single-threaded. Multiple threads can access to Supervisor. Each thread couldn't access at a same time but it includes memory-visibility problem.
Or, how about marking those vals as volatile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm......because supervisor is implemented as an actor, n and hiccups are maintained as the state of the actor and are only accessed via the handler of the message... so...I don't think it can be accessed by multiple threads ...I missed something in the code?

Copy link
Member

Choose a reason for hiding this comment

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

You try to log the current thread name in receive and then, you can see multiple threads access receive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean....yes, you're correct, since the running thread of the actor can be changed before the updated value is written back to the memory....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @sarutak , I went back to Akka's document http://doc.akka.io/docs/akka/snapshot/general/jmm.html (Actors and the Java Memory Model), I think they stated that,

"internal fields of the actor are visible when the next message is processed by that actor. So fields in your actor need not be volatile or equivalent. "

So, we don't need to explicitly mark these variables to be volatile?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, volatile is not necessary. https://groups.google.com/forum/#!msg/scalaz/kFnICLFjO-4/GT_59mZLrFAJ

Copy link
Member

Choose a reason for hiding this comment

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

I see, Akka's actor makes sure the visibility.

@CodingCat
Copy link
Contributor Author

anyone can take a review of this?

@CodingCat
Copy link
Contributor Author

ping

1 similar comment
@CodingCat
Copy link
Contributor Author

ping

@CodingCat
Copy link
Contributor Author

ping? @tdas do you mind taking a review?

@tdas
Copy link
Contributor

tdas commented Feb 23, 2015

Will do as soon as I finish some stuff regarding the Spark 1.3 release. :)

On Sun, Feb 22, 2015 at 7:55 AM, Nan Zhu notifications@github.com wrote:

ping? @tdas https://github.com/tdas do you mind take a review?

—
Reply to this email directly or view it on GitHub
#3984 (comment).

@CodingCat
Copy link
Contributor Author

sure, thanks

@srowen
Copy link
Member

srowen commented May 15, 2015

@tdas @CodingCat what's the status on this one? looks like it needs a rebase now

@CodingCat
Copy link
Contributor Author

I will do it next week....as there is a significant change in the code base (Akka was gone), I will think about how to adapt it to the current master branch

@SparkQA
Copy link

SparkQA commented May 18, 2015

Test build #33006 has finished for PR 3984 at commit 47de085.

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

@CodingCat
Copy link
Contributor Author

It's easier than I thought, the changes about RPC interface didn't touch this part

@tdas might have a review?

@tdas
Copy link
Contributor

tdas commented May 18, 2015

Sorry about the delay. Will take a look once the 1.4 release craze is over.
Soon. :)

On Mon, May 18, 2015 at 2:20 PM, Nan Zhu notifications@github.com wrote:

It's easier than I thought, the changes about RPC interface didn't touch
this part

@tdas https://github.com/tdas might have a review?

—
Reply to this email directly or view it on GitHub
#3984 (comment).

@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #38072 has finished for PR 3984 at commit 22fcad0.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #38073 has finished for PR 3984 at commit b020228.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • trait ActorHelper extends Logging

@SparkQA
Copy link

SparkQA commented Jul 22, 2015

Test build #1169 has finished for PR 3984 at commit b020228.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • trait ActorHelper extends Logging

@SparkQA
Copy link

SparkQA commented Jul 23, 2015

Test build #38138 has finished for PR 3984 at commit d0e06da.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class TrainValidationSplit(override val uid: String) extends Estimator[TrainValidationSplitModel]
    • trait ActorHelper extends Logging

@SparkQA
Copy link

SparkQA commented Jul 23, 2015

Test build #38140 has finished for PR 3984 at commit 30aad6e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class TrainValidationSplit(override val uid: String) extends Estimator[TrainValidationSplitModel]
    • trait ActorHelper extends Logging

@CodingCat
Copy link
Contributor Author

Hi, @tdas , do you have time to review it?

@zsxwing
Copy link
Member

zsxwing commented Dec 11, 2015

@CodingCat could you close this one? We can revisit this one when moving Actor to a separate project. Thanks!

@CodingCat
Copy link
Contributor Author

sure

@CodingCat CodingCat closed this Dec 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@CodingCat @SparkQA @tdas @srowen @zsxwing @markhamstra @sarutak