Re: Thrift IDL Feedback

[Suresh Marru <sm...@apache.org>]

Hi Eran,

Thanks for the detailed feedback. Please see more below:

On Feb 28, 2014, at 1:26 AM, Eran Chinthaka Withana <er...@gmail.com> wrote:

> Hi,
> 
> Here are my feedback.
> 
> 1. Thrift has a serious limitation handling nulls. For example, if you are
> trying to retrieve an experiment for a given id and if there is no
> experiment relating to that id, then you can not send null. To support
> this, we introduced a required boolean isEmpty for every data structure we
> defined in the thrift file. By default this is false but when we want to
> send null, we set this to true and send out the empty structure. So, may
> be, its a good idea to introduce this attribute if you are thinking of
> using any of these and either method inputs or return types.

I think this is a great idea. The experiment model has too much nested structure and it will be useful to have this check and ignore parsing through some of the objects. 

> 
> 2. If we define all the models in one file, then the maintenance of these
> struct could be an issue. There are couple of scenarios here
>  a) say, you want to introduce an API where you will only be using only
> few of these structs. But for that to work, you are *forced *to include the
> whole thrift file *AND *have all classes included in the jar file (unless
> you do selective exclusion of files).
>  b) this file can get bigger and bigger when there are more models to add
> 
> To solve this issues, we limited a single thrift file for a single data
> structure and have a version in it. But within the thrift file, it
> explicitly mentions the inclusions. Then, we can pass this to a tool (like
> Medusa: http://goo.gl/Og7IgF) and let it build self contained jars for a
> given data structure.
> Let me explain this a bit further. Lets say, you want to use Experiment
> struct and you want to create a jar out of ONLY the generated depending
> classes. With a tool like Medusa, it can read the thrift IDL, build the
> related other thrift files and build a jar out of only those files together
> with a related pom file. So rather than having a bulky all struct jar, you
> will have light weight jars.

I exactly intended to do this but refrained because of the jar building. But Medusa seems to be a good tool which will ease this task. So for the current release (0.12) will push ahead with this  monotonic thrift file, but will refactor it out for next release. Hopefully, wizzecommerce can open source medusa by then.

> 
> 3. Add documentation to each and every struct/enum. It may be clear to you
> know, but 6 months down the line you will forget what that is. Also, if you
> have better documentation, anyone can read and understand it (and may be
> generate thrift docs similar to java docs)

I was deferring this and already realizing some of the intentions are getting lost. We need to take this as a high priority.

> 
> 4. struct rootCauseErrorIdList is of type list. I'd try to use set instead
> of list for all primitive types as a good practice to avoid unnecessary
> duplicates whenever possible.

Good point changed the lists to sets. Will commit it after I change the server handler classes. 

This is very useful feedback. 

Thanks,
Suresh

> 
> 
> Thanks,
> Eran Chinthaka Withana
> 
> 
> On Tue, Feb 25, 2014 at 12:07 PM, Suresh Marru <sm...@apache.org> wrote:
> 
>> Hi All,
>> 
>> Sorry I have been distracted and could not catch up back on the object
>> database thread, will do so very soon. Meanwhile, would like to request
>> some feedback on the theft interfaces
>> 
>> Hi Eran,
>> 
>> The data model is near to complete for a first draft, but the API itself
>> is half-baked and the feedback you have below is already useful. I will
>> request for a API thrift IDL review later this week. Meanwhile, you have
>> any feedback on the data model thrift IDL -
>> https://git-wip-us.apache.org/repos/asf?p=airavata.git;a=blob_plain;f=airavata-api/thrift-interface-descriptions/experimentModel.thrift;hb=HEAD
>> 
>> Thanks,
>> Suresh
>> 
>> On Feb 24, 2014, at 1:40 AM, Eran Chinthaka Withana <
>> eran.chinthaka@gmail.com> wrote:
>> 
>>> Comments on thrift IDL
>>> 
>>> 1. The input and output parameters do not have constraint specifiers
>>> (required vs optional) and left to be default. This will be very
>>> challenging when we try to improve APIs in later versions and its a
>>> standard practise to ALWAYS have either optional or required as
>> constraint
>>> specifiers.
>>> 
>>> 2. consider using TypeDefs to reduce repetitive names. For example,
>>> defining airavataErrors.InvalidRequestException as a type will help you
>> to
>>> simply refer to that as InvalidRequestException
>>> 
>>> 3. Introduce a parameter for each method to get the API key. This will be
>>> helpful in the future to identify individual clients, enforce SLAs, logs
>>> requests, etc
>>