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>
>>> ------------------------------
>>
>>
