Thanks for the thorough review Kevin. I took the opportunity to quickly get the merge out of the way. This does not mean that I am not accepting further feedback and I will address the feedback you already provided now in master.
For the record, a long lived branch can be painful to maintain, but for a feature that can destabilize master I still think it is the way to go (maybe now that it is merged, it doesn’t seem so bad). Sumit. On 2/19/15, 5:40 PM, "Kevin Minder" <[email protected]> wrote: >Sumit, > >I went through it fairly carefully and I didn¹t find anything which >concerned me terribly. I like the direction and the implementation. >I left a bunch of line item comments in >https://issues.apache.org/jira/browse/KNOX-481, but I know you have been >suffering tracking master. >So I¹m +1 on a merge with a followup change to address my ³cleanup² level >feedback. > >+1 for merge. > >Kevin. > >On 2/18/15, 9:28 AM, "larry mccay" <[email protected]> wrote: > >>Cool - thanks, Sumit! >> >>On Wed, Feb 18, 2015 at 9:12 AM, Sumit Gupta >><[email protected]> >>wrote: >> >>> Sorry it seems that the server stripped my attachment, possibly because >>>I >>> named it poorly. >>> >>> Here it is (hopefully). Also if you would like to create a patch >>>locally >>> to use, you can checkout the branch KNOX-481 and then use: >>> >>> git format-patch master --stdout > KNOX-nnn.patch >>> >>> >>> >>> On 2/17/15, 11:57 PM, "Sumit Gupta" <[email protected]> >>>wrote: >>> >>> >So I tried attaching the big and ugly patch (due to all the commits >>>and >>> >merges that I failed to squash) and got rejected by the mail server >>>due to >>> >the size of the attachment. >>> > >>> >I have however attached a plain 'git diff' from master which is >>> >hopefully a lot easier to read. Let me know if something else will >>>make it >>> >easier. The branch itself of course is pushed and may just be easy >>>enough >>> >to look at in your IDE. >>> > >>> >Sumit. >>> > >>> > >>> > >>> >On 2/17/15, 10:55 PM, "larry mccay" <[email protected]> wrote: >>> > >>> >>This sounds like an excellent step forward for the deployment >>>machinery >>> >>of >>> >>Knox - good work! >>> >>It will also provide even greater compatibility with Hadoop platform >>> >>deployments. >>> >> >>> >>We should continue this work to define collections of versioned >>>services >>> >>as >>> >>a single Hadoop platform definition - composites. >>> >>I would also like to see providers referenced as reusable >>>configurations >>> >>of >>> >>security policy as we have discussed offline. >>> >> >>> >>I will spend some time reviewing this work tomorrow. >>> >>Is there a single patch available for the review? >>> >> >>> >>On Tue, Feb 17, 2015 at 10:33 PM, Sumit Gupta >>> >><[email protected]> >>> >>wrote: >>> >> >>> >>> I'm looking for comments, feedback and essentially a review of the >>> >>>branch >>> >>> KNOX-481. It contains work related to the jira of the same name. >>> >>> >>> >>> At the highest level, the changes involve removing boiler plate >>>code >>> >>>for >>> >>> adding a service that involved deployment and dispatch contributors >>>and >>> >>> replacing it by writing an xml file. The file needs to be called >>> >>> 'service.xml' (in this initial version) and be placed alongside the >>> >>> corresponding rewrite.xml file if there is one. >>> >>> >>> >>> The existing service 'definitions' have been converted to use this >>> >>>format >>> >>> and can be found in a new maven module called >>> >>>gateway-service-definitions. >>> >>> Just to recap, these services are: >>> >>> >>> >>> 1. Hbase >>> >>> 2. Hive >>> >>> 3. Oozie >>> >>> 4. Webhcat >>> >>> 5. Webhdfs >>> >>> 6. Yarn >>> >>> >>> >>> The structure of the files (and the files themselves) also make >>>their >>> >>>way >>> >>> into in the knox assembly under the 'data' directory. >>> >>> >>> >>> Without delving into the xml file too much, I am hoping that the >>> >>>existing >>> >>> service files provide enough of an example as does the fake service >>>for >>> >>>the >>> >>> test case. >>> >>> >>> >>> Other than that, some minor things of note are: >>> >>> >>> >>> 1. The httpclient is now shared a bit more than it was >>>previously >>> >>>(it >>> >>> was created for each request before) >>> >>> 2. The service definitions can now be versioned and the topology >>>can >>> >>> specify a version. If none is specified, the latest should be >>>picked. >>> >>> 3. The dispatch class can still be customized either by >>>specifying a >>> >>> contributor or a class name. >>> >>> 4. To make the writing of a custom dispatch easier, there is a >>> >>> configuration injection framework (thanks to Kevin Minder) that >>>will >>> >>>inject >>> >>> objects onto the dispatch instance from FilterConfig or >>>ServletContext. >>> >>> >>> >>> Sumit >>> >>> >>> > >>> >>> >
