Steve, I saw your changes posted https://reviews.apache.org/r/66319/diff/1/?page=5 here are my observations
1. This approach removes the thrift generated files from the repository. 2. This approach expects that contributors who are building sentry should have thrift installed. Please confirm. *Thanks,Kalyan Kumar Kalvagadda* | Software Engineer t. (469) 279- <0000000000>5732 cloudera.com <https://www.cloudera.com> [image: Cloudera] <https://www.cloudera.com/> [image: Cloudera on Twitter] <https://twitter.com/cloudera> [image: Cloudera on Facebook] <https://www.facebook.com/cloudera> [image: Cloudera on LinkedIn] <https://www.linkedin.com/company/cloudera> ------------------------------ On Wed, Mar 28, 2018 at 4:27 PM, Stephen Moist <mo...@cloudera.com> wrote: > Cool, interested in seeing how this works. > > > On Mar 28, 2018, at 2:21 PM, Kalyan Kumar Kalvagadda < > kkal...@cloudera.com> wrote: > > > > Steve, > > > > I found the solution for issue you reported where all the the > > auto-generated files are updated regardless of the change. > > we were not alone on this. This issue is seen because of the date that > was > > added in the auto generated files. > > > > Using a thrift option "generated_annotations=undated" we can avoid the > date > > from being added in to generated files. I will make a change in master > that > > will remove the date from all the thrift generated files. This will avoid > > this issue when some changes the thrift files. > > > > On Wed, Mar 28, 2018 at 1:56 PM, Stephen Moist <mo...@cloudera.com> > wrote: > > > >>>> 1. All the the thrift auto generated files getting updates regardless > >> of > >>>> what you change in the thrift definition. > >>>> - I'm sure there should way be a way to avoid that by making changes > >> to > >>>> the maven plug-in. we need to look into that. > >> There isn’t any thing in thrift that does a before/after comparison or > >> only makes changes based on new/removed things. It fully generates the > API > >> every time. > >> > >>> I agree with Sash that we rarely changes thrift API. Can we have a > tool, > >>> such as git, to compare the new and old generated file, and > automatically > >>> revert files not changed? > >> It’s an interesting idea. However, the tool would need to be able to > >> parse Thrift schema, know what it’ll be generating, parse git diff, > >> understand cosmetic non-functional changes(that we have the issue with > now) > >> in the diff, run git checkout — <file> commands on the files and then > more > >> importantly be able to detect the actual changes generated in files and > not > >> skip those. This is far more complex than what I’d like for a solution. > >> This would be a full feature to develop. > >> > >>> On Mar 28, 2018, at 12:24 PM, Na Li <lina...@cloudera.com> wrote: > >>> > >>> Steve, > >>> > >>> I agree with Sash that we rarely changes thrift API. Can we have a > tool, > >>> such as git, to compare the new and old generated file, and > automatically > >>> revert files not changed? If so, then that will have benefits of both > >> sides. > >>> I suspect the thrift version is tied to other components using thrift > >> too. > >>> It is easier to only have one version of thrift in the whole system. > >>> > >>> Thanks, > >>> > >>> Lina > >>> > >>> On Wed, Mar 28, 2018 at 11:53 AM, Kalyan Kumar Kalvagadda < > >>> kkal...@cloudera.com> wrote: > >>> > >>>> Steve, > >>>> > >>>> I agree with Sasha. > >>>> I see two concerns that you raised > >>>> > >>>> 1. All the the thrift auto generated files getting updates regardless > >> of > >>>> what you change in the thrift definition. > >>>> - I'm sure there should way be a way to avoid that by making changes > >> to > >>>> the maven plug-in. we need to look into that. > >>>> 2. Using latest version of thrift. > >>>> - We need to understand what we are getting into. we need do some > >>>> diligence to understand what what does it mean to us and take a > >> call > >>>> on > >>>> that. > >>>> > >>>> > >>>> -Kalyan > >>>> > >>>> > >>>> > >>>> On Wed, Mar 28, 2018 at 9:34 AM, Stephen Moist <mo...@cloudera.com> > >> wrote: > >>>> > >>>>> I mentioned earlier that Hadoop requires protobuf to be installed. > It > >> is > >>>>> a little inconvenient but it’s a 1 time setup. We can add a bit more > >>>>> documentation with all the full steps on how to install it and where > to > >>>> get > >>>>> it. I’ve worked on projects before in the past where I need mysql > setup > >>>> and > >>>>> running. I work on the Hadoop KMS/keyprovider and I have to modify > my > >>>> JRE > >>>>> by installing the Unlimited JCE jars. So at least in my experience, > >>>> there > >>>>> has always been some external dependency required before I can start > >>>>> developing. It’s annoying, but not a problem as long as the > >> instructions > >>>>> are clear and complete. > >>>>> > >>>>>> This also may confuse IDEs people use (since there are files that > >> needs > >>>>> to be built in order to be seen. > >>>>> I’m not sure about most, but I’d expect nearly all devs to do a build > >> of > >>>>> the project as soon as they check it out. Then it will generate > those > >>>>> files. The thrift generation is now part of the default mvn profile > so > >>>> they > >>>>> can’t not not have it generated for a full build. > >>>>> > >>>>> As far as using 0.9.3, is there a reason as to why we’re on this > >> version? > >>>>> Can we update to a later release? > >>>>> > >>>>>> On Mar 27, 2018, at 7:51 PM, Alexander Kolbasov <ak...@cloudera.com > > > >>>>> wrote: > >>>>>> > >>>>>> I have some concerns there. It is, indeed, a problem every time you > >>>> need > >>>>> to > >>>>>> do thrift changes, but so fat this was usually a rare event. Having > >>>>> thrift > >>>>>> as a requirement makes life inconvenient - for example the thrift > >>>> version > >>>>>> that I have on my laptop is 0.11.0 and I have to do something > special > >>>> to > >>>>>> get outdated thrift 0.9.3. This also may confuse IDEs people use > >> (since > >>>>>> there are files that needs to be built in order to be seen. > >>>>>> > >>>>>> On the plus side all thrift changes will work automatically and > people > >>>>>> wouldn't incidentally apply changes to thrift files. > >>>>>> > >>>>>> On Tue, Mar 27, 2018 at 1:34 PM, Stephen Moist <mo...@cloudera.com> > >>>>> wrote: > >>>>>> > >>>>>>> Looks like it about 5 seconds. > >>>>>>> > >>>>>>> org.apache.thrift.tools:maven-thrift-plugin > >>>>>>> > >>>>>>>> On Mar 27, 2018, at 3:27 PM, Sergio Pena < > sergio.p...@cloudera.com> > >>>>>>> wrote: > >>>>>>>> > >>>>>>>> Thanks Stephen for stepping into this issue. I've seen this issue > >>>>> before > >>>>>>>> and it is tedious to manually roll back each file. But because I > >>>> don't > >>>>>>>> generate thrift sources too often, then I didn't care at that > time. > >>>>>>>> > >>>>>>>> Anyway, thrift apis are in source control to the time generating > >> them > >>>>> in > >>>>>>>> every compile I assume. > >>>>>>>> How much extra time we'll have if we send them to the target/ > >>>> directory > >>>>>>> and > >>>>>>>> generate them on every compilation? > >>>>>>>> > >>>>>>>> What maven plugin are you talking about? > >>>>>>>> > >>>>>>>> On Tue, Mar 27, 2018 at 1:59 PM, Stephen Moist < > mo...@cloudera.com> > >>>>>>> wrote: > >>>>>>>> > >>>>>>>>> Hey all, I’ve started writing some skeleton apis for ABAC. In > the > >>>>>>>>> process, I’ve had to regenerate the Thrift api. Anytime that > >>>> happens, > >>>>>>> it > >>>>>>>>> regenerates all the files and gives it a new timestamp. So when > I > >>>>> look > >>>>>>> in > >>>>>>>>> git, it shows 75+ files have changed in addition to my new set of > >>>>>>> apis. I > >>>>>>>>> then have to manually roll back each file. As a development > >> process > >>>>>>> this > >>>>>>>>> is terrible. So… > >>>>>>>>> > >>>>>>>>> 1) Is there any reason why we need thrift apis in source control? > >>>>>>>>> 2) Anyone object to using a newer maven plugin to generate the > >>>> thrift > >>>>>>> apis > >>>>>>>>> on build time? > >>>>>>>>> 2a) This requires all developers to install thrift 0.9.3. I > >>>>> personally > >>>>>>>>> don’t see a problem as I have to have protobuf installed to > compile > >>>>>>>>> hadoop. (Most sentry devs probably already have it anyway) > >>>>>>>>> 2b) This will impact build systems to have thrift compiler > >>>> installed. > >>>>>>>>> 3) This prevents the above headache and makes it far simpler and > >>>>> faster > >>>>>>> to > >>>>>>>>> develop for Sentry. And makes for happier devs. > >>>>>>>>> 4) Dumping generated code into target/generated-sources is a > >>>> standard > >>>>>>>>> development practice when working against a known schema. > >>>>>>> > >>>>>>> > >>>>> > >>>>> > >>>> > >>>> > >>>> -- > >>>> *Kalyan Kumar Kalvagadda* | Software Engineer > >>>> t. (469) 279- <0000000000>5732 > >>>> cloudera.com <https://www.cloudera.com> > >>>> > >>>> [image: Cloudera] <https://www.cloudera.com/> > >>>> > >>>> [image: Cloudera on Twitter] <https://twitter.com/cloudera> [image: > >>>> Cloudera on Facebook] <https://www.facebook.com/cloudera> [image: > >> Cloudera > >>>> on LinkedIn] <https://www.linkedin.com/company/cloudera> > >>>> ------------------------------ > >>>> > >> > >> > > > > > > -- > > *Kalyan Kumar Kalvagadda* | Software Engineer > > t. (469) 279- <0000000000>5732 > > cloudera.com <https://www.cloudera.com> > > > > [image: Cloudera] <https://www.cloudera.com/> > > > > [image: Cloudera on Twitter] <https://twitter.com/cloudera> [image: > > Cloudera on Facebook] <https://www.facebook.com/cloudera> [image: > Cloudera > > on LinkedIn] <https://www.linkedin.com/company/cloudera> > > ------------------------------ > >
