-
Notifications
You must be signed in to change notification settings - Fork 159
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
Enhance Storm-YRAN for exeution on secure YARN Grid #31
Conversation
@liningsun tried out the latest code on his CDH 4.3 cluster. It worked nice. Storm master was launched with Nimbus, UI and supervisors. Then Storm topology was deployed successfully in the newly launched cluster. |
String storm_zip_path = (String)storm_conf.get("storm.zip.path"); | ||
Path zip = new Path(storm_zip_path); | ||
FileSystem fs = FileSystem.get(this.hadoopConf); | ||
localResources.put("storm", Util.newYarnAppResource(fs, zip, | ||
LocalResourceType.ARCHIVE, LocalResourceVisibility.PUBLIC)); | ||
LocalResourceType.ARCHIVE, LocalResourceVisibility.PRIVATE)); |
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.
The reason behind the PUBLIC was to make it so that the distributed cache would share instances of the .zip file between different users and storm clusters on the same box. The issue is that if it is not world readable it cannot be public. We should add in a piece of code to automatically check to see if the zip file and all of the parent directories all the way up are world readable too. If so we mark it as public if not we fall back to PRIVATE. This should probably be part of Util.newYarnAppResource. We may want to do it as a separate issue/pull request unless you can do it fast.
I just had the one comment. The code looks fine as is, but I think we want to create an issue for the public storm.zip. I also am a bit nervous about hadoop-0.23, hadoop-2.1 and master diverging, but that may be a separate issue to look into. |
We make 2 changes to address Bobby's point on storm.zip visibility:
|
//download storm.yaml file | ||
String storm_yaml_output = cl.getOptionValue("stormConfOutput"); | ||
if (storm_yaml_output!=null && storm_yaml_output.length()>0) { | ||
//try to download stirm.yaml |
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.
Minor typo s/stirm/storm/
Sorry I looked more closely at a few things. Most of what I found is minor, but the stopAllSupervisors I think is important to fix. |
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. See accompanying LICENSE file. | ||
--> |
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.
Would be nice to restore the formatting of the copyright notice in pom-0.23.xml. I think the rest of the XML has been reviewed with a non-white-space diff.
All issues have been fixed. I created a new issue for visibility detection, and will work on it later |
Looks good to me. |
Enhance Storm-YARN for execution on secure YARN Grid
This pull request is created so that Storm-YARN could be executed on secure YARN grid. In such a Grid, we need to:
Here is the basic steps to apply storm-yarn in secure grid: