Skip to content

[SPARK-7863][Core] Create SimpleDateFormat for every SimpleDateParam instance because it's not thread-safe #6406

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
Closed
zsxwing wants to merge 3 commits into from
Closed

[SPARK-7863][Core] Create SimpleDateFormat for every SimpleDateParam instance because it's not thread-safe #6406

zsxwing wants to merge 3 commits into from

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented May 26, 2015

SimpleDateFormat is not thread-safe. This PR creates new SimpleDateFormat for each SimpleDateParam instance.

srowen
srowen reviewed May 26, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/status/api/v1/SimpleDateParam.scala Outdated
Copy link
Member

@srowen srowen May 26, 2015

Choose a reason for hiding this comment

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

I wouldn't bother with ThreadLocal. Just make new objects on demand. These aren't expensive.

Copy link
Member Author

@zsxwing zsxwing May 26, 2015

Choose a reason for hiding this comment

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

I just feel using ThreadLocal is easy and minimizes the changes.

Copy link
Member

@srowen srowen May 26, 2015

Choose a reason for hiding this comment

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

It also means you keep per-thread state lying around. I know, it's not big, but I don't think it's worth it.

@SparkQA
Copy link

SparkQA commented May 26, 2015

Test build #33511 has finished for PR 6406 at commit 9711593.

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

@SparkQA
Copy link

SparkQA commented May 26, 2015

Test build #33509 timed out for PR 6406 at commit 0a3153c after a configured wait of 150m.

@zsxwing zsxwing changed the title [SPARK-7863][Core] Use ThreadLocal to protect SimpleDateFormat which is not thread-safe and fix a test [SPARK-7863][Core] Create SimpleDateFormat for every SimpleDateFormat instance because it's not thread-safe May 26, 2015
@zsxwing zsxwing changed the title [SPARK-7863][Core] Create SimpleDateFormat for every SimpleDateFormat instance because it's not thread-safe [SPARK-7863][Core] Create SimpleDateFormat for every SimpleDateParam instance because it's not thread-safe May 26, 2015
@zsxwing
Copy link
Member Author

zsxwing commented May 26, 2015

@srowen updated as per your suggestion

srowen
srowen reviewed May 26, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/status/api/v1/SimpleDateParam.scala Outdated
Copy link
Member

@srowen srowen May 26, 2015

Choose a reason for hiding this comment

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

Now this is a field though. There's no need for that. The format objects can be created on demand to parse the input. If this SimpleDateParam object is short-lived I suppose it does not matter much but I would have just inlined all this.

Copy link
Member Author

@zsxwing zsxwing May 26, 2015

Choose a reason for hiding this comment

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

Inlined formats.

srowen
srowen reviewed May 26, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/status/api/v1/SimpleDateParam.scala Outdated
Copy link
Member

@srowen srowen May 26, 2015

Choose a reason for hiding this comment

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

OK... but this seems like a very complicated way of saying, "parse with one format, and if it fails, parse with the other. Why not just boil this down while we're here? you can even avoid allocating at least one of the two objects in most cases.

@SparkQA
Copy link

SparkQA commented May 26, 2015

Test build #33517 has finished for PR 6406 at commit 49d08c6.

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

@SparkQA
Copy link

SparkQA commented May 26, 2015

Test build #33518 has finished for PR 6406 at commit 9680a15.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • implicit class DslLogicalPlan(val logicalPlan: LogicalPlan)

@SparkQA
Copy link

SparkQA commented May 26, 2015

Test build #33521 has finished for PR 6406 at commit 8cdd986.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • implicit class DslLogicalPlan(val logicalPlan: LogicalPlan)

andrewor14
andrewor14 reviewed May 26, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/status/api/v1/SimpleDateParam.scala Outdated
Copy link
Contributor

@andrewor14 andrewor14 May 26, 2015

Choose a reason for hiding this comment

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

why not just make this a def makeFormats or something and add a comment explaining why it needs to be a def? It would be good to not inline it because the old code is more readable.

@zsxwing
Copy link
Member Author

zsxwing commented May 26, 2015

Rewrote SimpleDateParam to avoid parsing twice

@SparkQA
Copy link

SparkQA commented May 27, 2015

Test build #33547 has finished for PR 6406 at commit aeed4c1.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member Author

zsxwing commented May 27, 2015

retest this please

@SparkQA
Copy link

SparkQA commented May 27, 2015

Test build #33551 has finished for PR 6406 at commit aeed4c1.

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

@srowen
Copy link
Member

srowen commented May 27, 2015

This LGTM.

@asfgit asfgit closed this in 8db40f6 May 29, 2015
@zsxwing zsxwing deleted the SPARK-7863 branch May 29, 2015 23:47
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
@zsxwing @jeanlyn
[SPARK-7863] [CORE] Create SimpleDateFormat for every SimpleDateParam…
6aa8190 
… instance because it's not thread-safe

SimpleDateFormat is not thread-safe. This PR creates new `SimpleDateFormat` for each `SimpleDateParam` instance.

Author: zsxwing <zsxwing@gmail.com>

Closes apache#6406 from zsxwing/SPARK-7863 and squashes the following commits:

aeed4c1 [zsxwing] Rewrite SimpleDateParam
8cdd986 [zsxwing] Inline formats
9680a15 [zsxwing] Create SimpleDateFormat for each SimpleDateParam instance because it's not thread-safe
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
@zsxwing @nemccarthy
[SPARK-7863] [CORE] Create SimpleDateFormat for every SimpleDateParam…
6db19c7 
… instance because it's not thread-safe

SimpleDateFormat is not thread-safe. This PR creates new `SimpleDateFormat` for each `SimpleDateParam` instance.

Author: zsxwing <zsxwing@gmail.com>

Closes apache#6406 from zsxwing/SPARK-7863 and squashes the following commits:

aeed4c1 [zsxwing] Rewrite SimpleDateParam
8cdd986 [zsxwing] Inline formats
9680a15 [zsxwing] Create SimpleDateFormat for each SimpleDateParam instance because it's not thread-safe
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

Development

Successfully merging this pull request may close these issues.

4 participants

@zsxwing @SparkQA @srowen @andrewor14