-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-1097] Workaround Hadoop conf ConcurrentModification issue #1273
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
Conversation
|
Merged build triggered. |
|
Merged build started. |
|
as described in #1000 |
|
Merged build finished. All automated tests passed. |
|
All automated tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use "conf" here and on the next line, instead of broadcastedConf.value.value? This is actually the first guy that assumes conf is not null, though, maybe add an assert for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aarondav , Yes, I also thought about this before. The reason I keep use broadcastedConf.value.value here is because: though broadcast variable is suggested to be read only and not changed, But I wonder maybe in case someone miss use it and change the value, read the latest value might be helpful. And it read the latest code in the next line in the original code, so I keep this style. But think again, if the value did changed in some place without hold any synchronize lock, this might still not be able to solve the problem. I will update the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regard conf , it is wrapped in SerializableWritable which enforce it to be a Writable, I think it won't be a null , or exception already been thrown somewhere before here. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep without the check, the NPE should be apparent on the synchronized block.
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. All automated tests passed. |
|
All automated tests passed. |
|
LGTM, merging into master and branch-1.0. |
Workaround Hadoop conf ConcurrentModification issue Author: Raymond Liu <[email protected]> Closes #1273 from colorant/hadoopRDD and squashes the following commits: 994e98b [Raymond Liu] Address comments e2cda3d [Raymond Liu] Workaround Hadoop conf ConcurrentModification issue (cherry picked from commit 5fa0a05) Signed-off-by: Aaron Davidson <[email protected]>
Workaround Hadoop conf ConcurrentModification issue Author: Raymond Liu <[email protected]> Closes apache#1273 from colorant/hadoopRDD and squashes the following commits: 994e98b [Raymond Liu] Address comments e2cda3d [Raymond Liu] Workaround Hadoop conf ConcurrentModification issue
Workaround Hadoop conf ConcurrentModification issue