Hi, Sorry about that bernd! :s/ben/bernd/g
-- ankur Sent from my iPhone > On Nov 4, 2014, at 3:13 AM, Bernd Mathiske <be...@mesosphere.io> wrote: > > Hi Ankur, > > I am Bernd, not Ben, but I’ll try to do my best :-) > > Your plan looks good to me and your patch for MESOS-1711 seems uncomplicated > enough to not cause me major problems no matter when it lands. > > Bernd > >> On Nov 4, 2014, at 11:44 AM, Ankur Chauhan <an...@malloc64.com> wrote: >> >> Hi ben, >> >> Thanks for the follow up. I think thats a perfectly fine game plan. I think >> I can break down things into smaller, more isolated chunks. But from the >> looks of MESOS-1316 the invocation code and the testing code seems to change >> a lot (in a good way), so to avoid a bunch of wasted cycles, I was going to >> hold off a little till it is committed and then go on to refactoring the >> transport parts. The json config change (MESOS-1248) is completely >> disconnected to this so I am not too worried about that one. >> The only place where some coordination may be required is the >> launcher/fetcher.cpp. It seems there are common changes happening between my >> change https://reviews.apache.org/r/27483/ >> <https://reviews.apache.org/r/27483/><https://reviews.apache.org/r/27483/ >> <https://reviews.apache.org/r/27483/>> and your >> https://reviews.apache.org/r/21316 >> <https://reviews.apache.org/r/21316><https://reviews.apache.org/r/21316 >> <https://reviews.apache.org/r/21316>> which would mean a little rework but >> it's totally within reasonable limits. >> I think if you get the MESOS-1316 committed and I get MESOS-1711 ( >> https://reviews.apache.org/r/27483 <https://reviews.apache.org/r/27483> >> <https://reviews.apache.org/r/27483 <https://reviews.apache.org/r/27483>> ) >> in, both of us would get unblocked in a way that we can get a way that lets >> us concurrently work on MESOS-336 and refactoring fetcher into more testable >> parts. >> >> Does this make sense to you or did I misunderstand some part? >> >> -- Ankur >> >>> On 4 Nov 2014, at 02:05, Bernd Mathiske <be...@mesosphere.io >>> <mailto:be...@mesosphere.io>> wrote: >>> >>> Typo: not -> note >>> >>>> On Nov 4, 2014, at 10:59 AM, Bernd Mathiske <be...@mesosphere.io >>>> <mailto:be...@mesosphere.io>> wrote: >>>> >>>> Hi Ankur, >>>> >>>> I like it, too. However, I cannot refrain from relaying (not my choice of >>>> word here) to you the advice to break your relatively large patch down >>>> into smaller parts. My patch for MESOS-336 certainly was, as I know now. >>>> My plan is to get MESOS-1316 (which is now under review) and MESOS-1248 >>>> (which should be quick) committed and then rebase MESOS-336 in smaller >>>> pieces. If you have small pieces, too, I do think we can do this somewhat >>>> concurrently, as your refactoring seems to affect mostly how the transport >>>> part works. I am on the other hand mostly interested in the bookkeeping of >>>> what is in progress, what succeeded, what failed, what has been put where, >>>> and so on, which can be separated, if we are careful about it. >>>> >>>> (BTW, I think we need both unit tests and integration tests. And we don’t >>>> have nearly enough of either.) >>>> >>>> Bernd >>>> >>>>> On Nov 3, 2014, at 6:18 PM, Adam Bordelon <a...@mesosphere.io >>>>> <mailto:a...@mesosphere.io><mailto:a...@mesosphere.io >>>>> <mailto: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 >>>>> <mailto:dha...@twopensource.com><mailto:dha...@twopensource.com >>>>> <mailto: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 >>>>> <mailto:an...@malloc64.com><mailto:an...@malloc64.com >>>>> <mailto: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 >>>>> <https://github.com/ankurcha/mesos/compare/prefer_2><https://github.com/ankurcha/mesos/compare/prefer_2 >>>>> <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 >>>>>> <mailto:an...@malloc64.com><mailto:an...@malloc64.com >>>>>> <mailto: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 >>>>>>> <mailto:a...@mesosphere.io><mailto:a...@mesosphere.io >>>>>>> <mailto: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 >>>>>>> <https://issues.apache.org/jira/browse/MESOS-1711 >>>>>>> <https://issues.apache.org/jira/browse/MESOS-1711>>; he may want to >>>>>>> review too. >>>>>>> >>>>>>> On Sat, Nov 1, 2014 at 8:49 PM, Ankur Chauhan <an...@malloc64.com >>>>>>> <mailto:an...@malloc64.com><mailto:an...@malloc64.com >>>>>>> <mailto:an...@malloc64.com>>> wrote: >>>>>>> Hi Tim, >>>>>>> >>>>>>> I just created a review https://reviews.apache.org/r/27483/ >>>>>>> <https://reviews.apache.org/r/27483/><https://reviews.apache.org/r/27483/ >>>>>>> <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 >>>>>>>> <mailto:an...@malloc64.com><mailto:an...@malloc64.com >>>>>>>> <mailto: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 >>>>>>>>> <mailto:t...@mesosphere.io><mailto:t...@mesosphere.io >>>>>>>>> <mailto: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 >>>>>>>>> <mailto:an...@malloc64.com><mailto:an...@malloc64.com >>>>>>>>> <mailto: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 >>>>>>>>>> <mailto:t...@mesosphere.io><mailto:t...@mesosphere.io >>>>>>>>>> <mailto: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 >>>>>>>>>>> <mailto:an...@malloc64.com><mailto:an...@malloc64.com >>>>>>>>>>> <mailto: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 >>>>>>>>>>> <https://issues.apache.org/jira/browse/MESOS-1711><https://issues.apache.org/jira/browse/MESOS-1711 >>>>>>>>>>> <https://issues.apache.org/jira/browse/MESOS-1711>> >>>>>>>>>>> >>>>>>>>>>> I have a branch ( >>>>>>>>>>> https://github.com/ankurcha/mesos/compare/prefer_hadoop_fetcher >>>>>>>>>>> <https://github.com/ankurcha/mesos/compare/prefer_hadoop_fetcher><https://github.com/ankurcha/mesos/compare/prefer_hadoop_fetcher >>>>>>>>>>> <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 >>>>>>>>>>>> <mailto:t...@duedil.com><mailto:t...@duedil.com >>>>>>>>>>>> <mailto: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-1887><https://issues.apache.org/jira/browse/MESOS-1887 >>>>>>>>>>>> <https://issues.apache.org/jira/browse/MESOS-1887>> >>>>>>>>>>>> - https://issues.apache.org/jira/browse/MESOS-1316 >>>>>>>>>>>> <https://issues.apache.org/jira/browse/MESOS-1316><https://issues.apache.org/jira/browse/MESOS-1316 >>>>>>>>>>>> <https://issues.apache.org/jira/browse/MESOS-1316>> >>>>>>>>>>>> - https://issues.apache.org/jira/browse/MESOS-336 >>>>>>>>>>>> <https://issues.apache.org/jira/browse/MESOS-336><https://issues.apache.org/jira/browse/MESOS-336 >>>>>>>>>>>> <https://issues.apache.org/jira/browse/MESOS-336>> >>>>>>>>>>>> - https://issues.apache.org/jira/browse/MESOS-1248 >>>>>>>>>>>> <https://issues.apache.org/jira/browse/MESOS-1248><https://issues.apache.org/jira/browse/MESOS-1248 >>>>>>>>>>>> <https://issues.apache.org/jira/browse/MESOS-1248>> >>>>>>>>>>>> >>>>>>>>>>>> On 31 October 2014 22:39, Tim Chen <t...@mesosphere.io >>>>>>>>>>>> <mailto:t...@mesosphere.io><mailto:t...@mesosphere.io >>>>>>>>>>>> <mailto: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 >>>>>>>>>>>> <mailto:an...@malloc64.com><mailto:an...@malloc64.com >>>>>>>>>>>> <mailto: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. >
