Skip to content

[SPARK-6197][CORE] handle json exception when hisotry file not finished writing#4927

Closed
liyezhang556520 wants to merge 3 commits intoapache:masterfrom
liyezhang556520:jsonParseError
Closed

[SPARK-6197][CORE] handle json exception when hisotry file not finished writing#4927
liyezhang556520 wants to merge 3 commits intoapache:masterfrom
liyezhang556520:jsonParseError

Conversation

@liyezhang556520
Copy link
Copy Markdown
Contributor

@liyezhang556520 liyezhang556520 commented Mar 6, 2015

For details, please refer to SPARK-6197

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 6, 2015

Test build #28328 has started for PR 4927 at commit 2973024.

  • This patch merges cleanly.

@liyezhang556520
Copy link
Copy Markdown
Contributor Author

liyezhang556520 commented Mar 6, 2015

cc @srowen, @andrewor14, @viirya

@viirya
Copy link
Copy Markdown
Member

viirya commented Mar 6, 2015

@liyezhang556520 do you really encounter this problem? Isn't the writing operation atomic?

@liyezhang556520
Copy link
Copy Markdown
Contributor Author

liyezhang556520 commented Mar 6, 2015

Yes, I do encounter the problem, if you use Ctrl+C during tasks finishing, it's easy to happen.

@zsxwing
Copy link
Copy Markdown
Member

zsxwing commented Mar 6, 2015

@liyezhang556520 do you really encounter this problem? Isn't the writing operation atomic?

The writing operation is not atomic.

@liyezhang556520
Copy link
Copy Markdown
Contributor Author

liyezhang556520 commented Mar 6, 2015

And I don't think writing operation is atomic, signals still can iterrupt

@liyezhang556520
Copy link
Copy Markdown
Contributor Author

liyezhang556520 commented Mar 6, 2015

ok, thanks @zsxwing answer the second question.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 6, 2015

Test build #28328 has finished for PR 4927 at commit 2973024.

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

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Mar 6, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28328/
Test PASSed.

srowen
srowen reviewed Mar 6, 2015
View reviewed changes
Comment thread core/src/main/scala/org/apache/spark/scheduler/ReplayListenerBus.scala Outdated
Copy link
Copy Markdown
Member

@srowen srowen Mar 6, 2015

Choose a reason for hiding this comment

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

Nit: let's call this maybeTruncated? it's not certain that the file is truncated.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 6, 2015

Test build #28344 has started for PR 4927 at commit 2b48831.

  • This patch merges cleanly.

@liyezhang556520
Copy link
Copy Markdown
Contributor Author

liyezhang556520 commented Mar 6, 2015

@srowen , thanks for your comments, and I have updated the code.

srowen
srowen reviewed Mar 6, 2015
View reviewed changes
Comment thread core/src/main/scala/org/apache/spark/deploy/master/Master.scala Outdated
Copy link
Copy Markdown
Member

@srowen srowen Mar 6, 2015

Choose a reason for hiding this comment

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

OK by me except this line no longer needs to wrap. Maybe see if others have a comment before bothering to update again.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 6, 2015

Test build #28345 has started for PR 4927 at commit 5cbdc82.

  • This patch merges cleanly.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 6, 2015

Test build #28344 has finished for PR 4927 at commit 2b48831.

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

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Mar 6, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28344/
Test PASSed.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 6, 2015

Test build #28345 has finished for PR 4927 at commit 5cbdc82.

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

@AmplabJenkins
Copy link
Copy Markdown

AmplabJenkins commented Mar 6, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28345/
Test PASSed.

@srowen
Copy link
Copy Markdown
Member

srowen commented Mar 6, 2015

Thoughts, @viirya @zsxwing ?

@viirya
Copy link
Copy Markdown
Member

viirya commented Mar 6, 2015

Hmm, just being curious, replay originally has a try...catch block outside the log reading, and this pr adds another one in the reading. Is it better to combine two try...catch blocks together for readability?

@liyezhang556520
Copy link
Copy Markdown
Contributor Author

liyezhang556520 commented Mar 6, 2015

@viirya , that is because we need to check whether lines iterator is exhausted (by checking lines.hasNext) to decide whether we can ignore the exception, and the lines is only available in the try block.

@viirya
Copy link
Copy Markdown
Member

viirya commented Mar 6, 2015

Ah, not notice that. :-)

zsxwing
zsxwing reviewed Mar 13, 2015
View reviewed changes
Comment thread core/src/main/scala/org/apache/spark/scheduler/ReplayListenerBus.scala
Copy link
Copy Markdown
Member

@zsxwing zsxwing Mar 13, 2015

Choose a reason for hiding this comment

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

The import order is wrong. See https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide#SparkCodeStyleGuide-Imports

@zsxwing
Copy link
Copy Markdown
Member

zsxwing commented Mar 13, 2015

LGTM except the minor import order problem.

@srowen
Copy link
Copy Markdown
Member

srowen commented Mar 13, 2015

I can fix the import on merge.

@asfgit asfgit closed this in 9048e81 Mar 13, 2015
zzcclp pushed a commit to zzcclp/spark that referenced this pull request Mar 16, 2015
@liyezhang556520 @zzcclp
[SPARK-6197][CORE] handle json exception when hisotry file not finish…
5fc818d 
…ed writing

For details, please refer to [SPARK-6197](https://issues.apache.org/jira/browse/SPARK-6197)

Author: Zhang, Liye <liye.zhang@intel.com>

Closes apache#4927 from liyezhang556520/jsonParseError and squashes the following commits:

5cbdc82 [Zhang, Liye] without unnecessary wrap
2b48831 [Zhang, Liye] small changes with sean owen's comments
2973024 [Zhang, Liye] handle json exception when file not finished writing

Conflicts:
	core/src/main/scala/org/apache/spark/deploy/master/Master.scala
@liyezhang556520 liyezhang556520 deleted the jsonParseError branch March 20, 2015 05:18
asfgit pushed a commit that referenced this pull request May 16, 2015
@liyezhang556520 @srowen
[SPARK-6197][CORE] handle json exception when hisotry file not finish…
91442fd 
…ed writing

For details, please refer to [SPARK-6197](https://issues.apache.org/jira/browse/SPARK-6197)

Author: Zhang, Liye <liye.zhang@intel.com>

Closes #4927 from liyezhang556520/jsonParseError and squashes the following commits:

5cbdc82 [Zhang, Liye] without unnecessary wrap
2b48831 [Zhang, Liye] small changes with sean owen's comments
2973024 [Zhang, Liye] handle json exception when file not finished writing
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.

6 participants

@liyezhang556520 @SparkQA @viirya @zsxwing @AmplabJenkins @srowen