Skip to content

[SPARK-1112, 2156] (1.0 edition) Use correct akka frame size and overhead amounts.#1172

Closed
pwendell wants to merge 2 commits into
apache:branch-1.0from
pwendell:akka-10-fix
Closed

[SPARK-1112, 2156] (1.0 edition) Use correct akka frame size and overhead amounts.#1172
pwendell wants to merge 2 commits into
apache:branch-1.0from
pwendell:akka-10-fix

Conversation

@pwendell

Copy link
Copy Markdown
Contributor

SPARK-1112: This is a more conservative version of #1132 that doesn't change
around the actor system initialization on the executor. Instead we just
directly read the current frame size limit from the ActorSystem.

SPARK-2156: This uses the same fixe as in #1132.

…head amounts.

SPARK-1112: This is a more conservative version of apache#1132 that doesn't change
around the actor system initialization on the executor. Instead we just
directly read the current frame size limit from the ActorSystem.

SPARK-2156: This uses the same fixe as in apache#1132.
@pwendell

Copy link
Copy Markdown
Contributor Author

@mengxr - do you mind reviewing this?

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished.

@AmplabJenkins

Copy link
Copy Markdown

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15998/

@mengxr

mengxr commented Jun 22, 2014

Copy link
Copy Markdown
Contributor

Jenkins, retest it please.

@mengxr

mengxr commented Jun 22, 2014

Copy link
Copy Markdown
Contributor

LGTM. I will merge it to branch-1.0 if Jenkins is happy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Slightly confused on this -- won't this make LocalBackend and MesosExecutorBackend never use the BlockManager for results, causing them to have the undesired behavior?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The MesosExecutorBackend sends results through mesos, not akka. The LocalBackend sends a message to an actor within the same actor system... which I assumed won't go over TCP.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see. So the only real change is that in certain cases the LocalBackend will no longer use the BlockManager for returning results. Sounds fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So that change actually alters the expectations of the unit tests, so I went ahead and just enforced the limit in the LocalBackend anwyays.

@pwendell

Copy link
Copy Markdown
Contributor Author

Jenkins, retest this please.

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished.

@AmplabJenkins

Copy link
Copy Markdown

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16007/

@mengxr

mengxr commented Jun 22, 2014

Copy link
Copy Markdown
Contributor

Jenkins, retest this please.

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished.

@AmplabJenkins

Copy link
Copy Markdown

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16010/

@pwendell

Copy link
Copy Markdown
Contributor Author

I think these are failing because our tests assume that in local mode we enforce the frame size limit (which we actually don't need to). I'll make the appropriate adjustments in a bit.

@pwendell

Copy link
Copy Markdown
Contributor Author

Jenkins, retest this please.

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished. All automated tests passed.

@AmplabJenkins

Copy link
Copy Markdown

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16014/

@pwendell

Copy link
Copy Markdown
Contributor Author

@aarondav mind taking a final pass and merging this?

@aarondav

Copy link
Copy Markdown
Contributor

Absolutely. LGTM, merging into branch-1.0.

asfgit pushed a commit that referenced this pull request Jun 23, 2014
…head amounts.

SPARK-1112: This is a more conservative version of #1132 that doesn't change
around the actor system initialization on the executor. Instead we just
directly read the current frame size limit from the ActorSystem.

SPARK-2156: This uses the same fixe as in #1132.

Author: Patrick Wendell <pwendell@gmail.com>

Closes #1172 from pwendell/akka-10-fix and squashes the following commits:

d56297e [Patrick Wendell] Set limit in LocalBackend to preserve test expectations
9f5ed19 [Patrick Wendell] [SPARK-1112, 2156] (1.0 edition) Use correct akka frame size and overhead amounts.
@aarondav

Copy link
Copy Markdown
Contributor

You may have to close this manually, @pwendell, I'm not sure github will close it if it's not in master.

@pwendell pwendell closed this Jun 23, 2014
@pwendell

Copy link
Copy Markdown
Contributor Author

Thanks, closed.

mengxr pushed a commit to mengxr/spark that referenced this pull request Jul 17, 2014
asfgit pushed a commit that referenced this pull request Jul 17, 2014
…head amounts.

backport #1172 to branch-0.9.

Author: Patrick Wendell <pwendell@gmail.com>

Closes #1455 from mengxr/akka-fix-0.9 and squashes the following commits:

a99f201 [Patrick Wendell] backport PR #1172 to branch-0.9
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