Yea, I saw those today morning. I'll hold off a little mesos-336 changes a lot of stuff.
Sent from my iPhone > On Nov 3, 2014, at 9:18 AM, Adam Bordelon <a...@mesosphere.io> wrote: > > + Bernd, who has done some fetcher work, including additional testing, for > MESOS-1316, MESOS-1945, and MESOS-336 > >> On Mon, Nov 3, 2014 at 9:04 AM, Dominic Hamon <dha...@twopensource.com> >> wrote: >> Hi Ankur >> >> I think this is a great approach. It makes the code much simpler, >> extensible, and more testable. Anyone that's heard me rant knows I am a big >> fan of unit tests over integration tests, so this shouldn't surprise anyone >> :) >> >> If you haven't already, please read the documentation on contributing to >> Mesos and the style guide to ensure all the naming is as expected, then you >> can push the patch to reviewboard to get it reviewed and committed. >> >>> On Mon, Nov 3, 2014 at 12:49 AM, Ankur Chauhan <an...@malloc64.com> wrote: >>> Hi, >>> >>> I did some learning today! This is pretty much a very rough draft of the >>> tests/refactor of mesos-fetcher that I have come up with. Again, If there >>> are some obvious mistakes, please let me know. (this is my first pass after >>> all). >>> https://github.com/ankurcha/mesos/compare/prefer_2 >>> >>> My main intention is to break the logic of the fetcher info some very >>> discrete components that i can write tests against. I am still re-learning >>> cpp/mesos code styles etc so I may be a little slow to catch up but I would >>> really appreciate any comments and/or suggestions. >>> >>> -- Ankur >>> @ankurcha >>> >>>> On 2 Nov 2014, at 18:17, Ankur Chauhan <an...@malloc64.com> wrote: >>>> >>>> Hi, >>>> >>>> I noticed that the current set of tests in `src/tests/fetcher_tests.cpp` >>>> is pretty coarse grained and are more on the lines of a functional test. I >>>> was going to add some tests but it seems like if I am to do that I would >>>> need to add a test dependency on hadoop. >>>> >>>> As an alternative, I propose adding a good set of unit tests around the >>>> methods used by `src/launcher/fetcher.cpp` and `src/hdfs/hdfs.cpp`. This >>>> should be able to catch a good portion of cases at the same time keeping >>>> the dependencies and runtime of tests low. What do you guys thing about >>>> this? >>>> >>>> PS: I am pretty green in terms of gtest and the overall c++ testing >>>> methodology. Can someone give me pointers to good examples of tests in the >>>> codebase. >>>> >>>> -- Ankur >>>> >>>>> On 1 Nov 2014, at 22:54, Adam Bordelon <a...@mesosphere.io> wrote: >>>>> >>>>> Thank you Ankur. At first glance, it looks great. We'll do a more >>>>> thorough review of it very soon. >>>>> I know Tim St. Clair had some ideas for fixing MESOS-1711; he may want to >>>>> review too. >>>>> >>>>>> On Sat, Nov 1, 2014 at 8:49 PM, Ankur Chauhan <an...@malloc64.com> wrote: >>>>>> Hi Tim, >>>>>> >>>>>> I just created a review https://reviews.apache.org/r/27483/ It's my >>>>>> first stab at it and I will try to add more tests as I figure out how to >>>>>> do the hadoop mocking and stuff. Have a look and let me know what you >>>>>> think about it so far. >>>>>> >>>>>> -- Ankur >>>>>> >>>>>>> On 1 Nov 2014, at 20:05, Ankur Chauhan <an...@malloc64.com> wrote: >>>>>>> >>>>>>> Yea, i saw that the minute i pressed send. I'll start the review board >>>>>>> so that people can have a look at the change. >>>>>>> >>>>>>> -- Ankur >>>>>>> >>>>>>>> On 1 Nov 2014, at 20:01, Tim Chen <t...@mesosphere.io> wrote: >>>>>>>> >>>>>>>> Hi Ankur, >>>>>>>> >>>>>>>> There is a fetcher_tests.cpp in src/tests. >>>>>>>> >>>>>>>> Tim >>>>>>>> >>>>>>>>> On Sat, Nov 1, 2014 at 7:27 PM, Ankur Chauhan <an...@malloc64.com> >>>>>>>>> wrote: >>>>>>>>> Hi Tim, >>>>>>>>> >>>>>>>>> I am trying to find/write some test cases. I couldn't find a >>>>>>>>> fetcher_tests.{cpp|hpp} so once I have something, I'll post on review >>>>>>>>> board. I am new to gmock/gtest so bear with me while i get up to >>>>>>>>> speed. >>>>>>>>> >>>>>>>>> -- Ankur >>>>>>>>> >>>>>>>>>> On 1 Nov 2014, at 19:23, Timothy Chen <t...@mesosphere.io> wrote: >>>>>>>>>> >>>>>>>>>> Hi Ankur, >>>>>>>>>> >>>>>>>>>> Can you post on reviewboard? We can discuss more about the code >>>>>>>>>> there. >>>>>>>>>> >>>>>>>>>> Tim >>>>>>>>>> >>>>>>>>>> Sent from my iPhone >>>>>>>>>> >>>>>>>>>>> On Nov 1, 2014, at 6:29 PM, Ankur Chauhan <an...@malloc64.com> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Tim, >>>>>>>>>>> >>>>>>>>>>> I don't think there is an issue which is directly in line with what >>>>>>>>>>> i wanted but the closest one that I could find in JIRA is >>>>>>>>>>> https://issues.apache.org/jira/browse/MESOS-1711 >>>>>>>>>>> >>>>>>>>>>> I have a branch ( >>>>>>>>>>> https://github.com/ankurcha/mesos/compare/prefer_hadoop_fetcher ) >>>>>>>>>>> that has a change that would enable users to specify whatever hdfs >>>>>>>>>>> compatible uris to the mesos-fetcher but maybe you can weight in on >>>>>>>>>>> it. Do you think this is the right track? if so, i would like to >>>>>>>>>>> pick this issue and submit a patch for review. >>>>>>>>>>> >>>>>>>>>>> -- Ankur >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> On 1 Nov 2014, at 04:32, Tom Arnfeld <t...@duedil.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Completely +1 to this. There are now quite a lot of hadoop >>>>>>>>>>>> compatible filesystem wrappers out in the wild and this would >>>>>>>>>>>> certainly be very useful. >>>>>>>>>>>> >>>>>>>>>>>> I'm happy to contribute a patch. Here's a few related issues that >>>>>>>>>>>> might be of interest; >>>>>>>>>>>> >>>>>>>>>>>> - https://issues.apache.org/jira/browse/MESOS-1887 >>>>>>>>>>>> - https://issues.apache.org/jira/browse/MESOS-1316 >>>>>>>>>>>> - https://issues.apache.org/jira/browse/MESOS-336 >>>>>>>>>>>> - https://issues.apache.org/jira/browse/MESOS-1248 >>>>>>>>>>>> >>>>>>>>>>>>> On 31 October 2014 22:39, Tim Chen <t...@mesosphere.io> wrote: >>>>>>>>>>>>> I believe there is already a JIRA ticket for this, if you search >>>>>>>>>>>>> for fetcher in Mesos JIRA I think you can find it. >>>>>>>>>>>>> >>>>>>>>>>>>> Tim >>>>>>>>>>>>> >>>>>>>>>>>>>> On Fri, Oct 31, 2014 at 3:27 PM, Ankur Chauhan >>>>>>>>>>>>>> <an...@malloc64.com> wrote: >>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>> >>>>>>>>>>>>>> I have been looking at some of the stuff around the fetcher and >>>>>>>>>>>>>> saw something interesting. The code for fetcher::fetch method is >>>>>>>>>>>>>> dependent on a hard coded list of url schemes. No doubt that >>>>>>>>>>>>>> this works but is very restrictive. >>>>>>>>>>>>>> Hadoop/HDFS in general is pretty flexible when it comes to being >>>>>>>>>>>>>> able to fetch stuff from urls and has the ability to fetch a >>>>>>>>>>>>>> large number of types of urls and can be extended by adding >>>>>>>>>>>>>> configuration into the conf/hdfs-site.xml and core-site.xml >>>>>>>>>>>>>> >>>>>>>>>>>>>> What I am proposing is that we refactor the fetcher.cpp to >>>>>>>>>>>>>> prefer to use the hdfs (using hdfs/hdfs.hpp) to do all the >>>>>>>>>>>>>> fetching if HADOOP_HOME is set and $HADOOP_HOME/bin/hadoop is >>>>>>>>>>>>>> available. This logic already exists and we can just use it. The >>>>>>>>>>>>>> fallback logic for using net::download or local file copy is may >>>>>>>>>>>>>> be left in place for installations that do not have hadoop >>>>>>>>>>>>>> configured. This means that if hadoop is present we can directly >>>>>>>>>>>>>> fetch urls such as tachyon://... snackfs:// ... cfs:// .... >>>>>>>>>>>>>> ftp://... s3://... http:// ... file:// with no extra effort. >>>>>>>>>>>>>> This makes up for a much better experience when it comes to >>>>>>>>>>>>>> debugging and extensibility. >>>>>>>>>>>>>> >>>>>>>>>>>>>> What do others think about this? >>>>>>>>>>>>>> >>>>>>>>>>>>>> - Ankur >> >> >> >> -- >> Dominic Hamon | @mrdo | Twitter >> There are no bad ideas; only good ideas that go horribly wrong. >
