Steve, I agree with Kalyan. Solving this issue by removing date from the generated files is easier for most people, and can achieve our goal that only files contain real changes are checked out.
Thanks, Lina On Wed, Apr 4, 2018 at 4:28 PM, Kalyan Kumar Kalvagadda < kkal...@cloudera.com> wrote: > Steve, > > This may not be a desirable behavior for couple of reasons > > 1. Asking every contributor to install and use a specific thrift version > is inconvenient when we know changes to thrift are not often. > 2. All the build servers that build sentry should be updated with thrift > 3. When we know there is a simple way to get around the issue(all the > thirft files getting updated regardless of the change) we don't have to > make a such a change. > > > -kalyan > > > *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, Apr 4, 2018 at 3:29 PM, Stephen Moist <mo...@cloudera.com> wrote: > > > Yes both points are correct. > > > > > On Apr 4, 2018, at 1:29 PM, Kalyan Kumar Kalvagadda < > > kkal...@cloudera.com> wrote: > > > > > > 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> > > >>> ------------------------------ > > >> > > >> > > > > >
