Skip to content

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Dec 2, 2015

SynchronousQueue cannot cache any task. This issue is similar to #9978. It's an easy fix. Just use the fixed ThreadUtils.newDaemonCachedThreadPool.

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous codes are right. Just use newDaemonCachedThreadPool to make codes simple.

@srowen
Copy link
Member

srowen commented Dec 2, 2015

This should be attached to SPARK-11999 right?

@zsxwing
Copy link
Member Author

zsxwing commented Dec 2, 2015

This should be attached to SPARK-11999 right?

RC1 is coming out, so I created a new ticket.

@srowen
Copy link
Member

srowen commented Dec 2, 2015

Hm, what do you mean? no RC has been announced

@zsxwing
Copy link
Member Author

zsxwing commented Dec 2, 2015

RC1 is cut. See bf52584

@srowen
Copy link
Member

srowen commented Dec 2, 2015

@pwendell this should probably be announced on the list when you start or else we might have a very small chance of a race condition in tagging/merging things.

I get the point that your first half of the fix could conceivably end up in 1.6.0 and not the other, but is that a good thing? you made a second JIRA anyway I see, so one of them needs to be associated here.

I think we have a larger problem, as ever in Spark, if people are just merging fixes right up to and well past the deadline. We were supposed to be cutting RCs 3 weeks ago, but it arbitrarily happened now.

@pwendell
Copy link
Contributor

pwendell commented Dec 2, 2015

Hey @marmbrus is actually managing the RC - it just has my name on it because some automated tooling uses my account. Ping @marmbrus.

@zsxwing zsxwing changed the title Fix thread pools that cannot cache tasks in Worker and AppClient [SPARK-12101][Core]Fix thread pools that cannot cache tasks in Worker and AppClient Dec 2, 2015
@pwendell
Copy link
Contributor

pwendell commented Dec 2, 2015

BTW @srowen, some protocol for announcing these is probably a good idea to avoid races. I think we haven't suffered from races in the past, but mostly out of luck.

@zsxwing
Copy link
Member Author

zsxwing commented Dec 2, 2015

I get the point that your first half of the fix could conceivably end up in 1.6.0 and not the other, but is that a good thing? you made a second JIRA anyway I see, so one of them needs to be associated here.

Oops. Sorry. Forgot to add the JIRA number to the title.

@marmbrus
Copy link
Contributor

marmbrus commented Dec 2, 2015

Sorry if this seemed arbitrary, I cut the release as soon as all the blocker issues were resolved. In the future I'll announce on the dev list first.

@SparkQA
Copy link

SparkQA commented Dec 2, 2015

Test build #47079 has finished for PR 10108 at commit 2b9f4da.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

the #%d doesn't do anything here right? We don't call format on this string

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, this is setting a format so it does do something. Maybe we should add it back in the new code

Copy link
Member Author

Choose a reason for hiding this comment

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

ThreadUtils.namedThreadFactory will do the similar thing except the format is -%d. I think it's better to make all threads have the same format in Spark.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I see, it's a prefix that we're passing in here

@andrewor14
Copy link
Contributor

This patch itself looks good pending 1 minor comment.

@andrewor14
Copy link
Contributor

Merging into master 1.6

@asfgit asfgit closed this in 649be4f Dec 3, 2015
asfgit pushed a commit that referenced this pull request Dec 3, 2015
…r and AppClient

`SynchronousQueue` cannot cache any task. This issue is similar to #9978. It's an easy fix. Just use the fixed `ThreadUtils.newDaemonCachedThreadPool`.

Author: Shixiong Zhu <[email protected]>

Closes #10108 from zsxwing/fix-threadpool.

(cherry picked from commit 649be4f)
Signed-off-by: Shixiong Zhu <[email protected]>
@zsxwing zsxwing deleted the fix-threadpool branch December 3, 2015 19:25
asfgit pushed a commit that referenced this pull request Dec 7, 2015
…r and AppClient (backport 1.5)

backport #10108 to branch 1.5

Author: Shixiong Zhu <[email protected]>

Closes #10135 from zsxwing/fix-threadpool-1.5.
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.

6 participants