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. 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. 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) 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. Thanks, Eran Chinthaka Withana On Tue, Feb 25, 2014 at 12:07 PM, Suresh Marru <[email protected]> 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 < > [email protected]> 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 >
