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.

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

[SPARK-6860][Streaming][WebUI] Fix the possible inconsistency of StreamingPage #5470

Closed
wants to merge 2 commits into from
Closed

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Apr 11, 2015

Because StreamingPage.render doesn't hold the listener lock when generating the content, the different parts of content may have some inconsistent values if listener updates its status at the same time. And it will confuse people.

This PR added listener.synchronized to make sure we have a consistent view of StreamingJobProgressListener when creating the content.

…obProgressListener when creating the content
@srowen
Copy link
Member

srowen commented Apr 11, 2015

That makes some sense. It depends on the methods that mutate the listener also being synchronized on the object. It looks like they are, at a glance, but have you looked into that a bit?

@SparkQA
Copy link

SparkQA commented Apr 11, 2015

Test build #30076 has finished for PR 5470 at commit 7182498.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 11, 2015

That makes some sense. It depends on the methods that mutate the listener also being synchronized on the object. It looks like they are, at a glance, but have you looked into that a bit?

Thank you for reminding me. Some public methods of StreamingJobProgressListener are not protected by synchronized. I will update them, too.

@SparkQA
Copy link

SparkQA commented Apr 11, 2015

Test build #30081 has finished for PR 5470 at commit cec6f92.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@srowen
Copy link
Member

srowen commented Apr 12, 2015

LGTM. The first commit fixes a problem that would indeed in rare cases cause the page to display an inconsistent view of the state, which is probably minor but still bad. The second synchronizes access to mutable data structures that were inconsistently synchronized.

@asfgit asfgit closed this in 14ce3ea Apr 13, 2015
@zsxwing zsxwing deleted the SPARK-6860 branch April 13, 2015 13:55
@zsxwing
Copy link
Member Author

zsxwing commented Apr 15, 2015

@srowen, do you think if this one should be merged to branch 1.3?

@srowen
Copy link
Member

srowen commented Apr 15, 2015

I would back-port if anyone felt strongly about it; it seems like a theoretical and cosmetic problem, so didn't seem high priority.

@zsxwing
Copy link
Member Author

zsxwing commented Apr 15, 2015

I would back-port if anyone felt strongly about it; it seems like a theoretical and cosmetic problem, so didn't seem high priority.

Fair. If necessary, I would help send a back-port pull request.

@srowen
Copy link
Member

srowen commented Apr 15, 2015

Meh, I'll just back port it. It's low risk.

asfgit pushed a commit that referenced this pull request Apr 15, 2015
…amingPage

Because `StreamingPage.render` doesn't hold the `listener` lock when generating the content, the different parts of content may have some inconsistent values if `listener` updates its status at the same time. And it will confuse people.

This PR added `listener.synchronized` to make sure we have a consistent view of StreamingJobProgressListener when creating the content.

Author: zsxwing <zsxwing@gmail.com>

Closes #5470 from zsxwing/SPARK-6860 and squashes the following commits:

cec6f92 [zsxwing] Add missing 'synchronized' in StreamingJobProgressListener
7182498 [zsxwing] Add synchronized to make sure we have a consistent view of StreamingJobProgressListener when creating the content
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants