See comments inline On Sat, Mar 1, 2014 at 5:09 PM, kishore g <[email protected]> wrote: > Awesome work, I am review the api package. Can you provide some additional > info on commons-discovery ?. Is this short term or long term, whats the > value add.
[Sandeep] We can/should remove this long term, right now it does the job of creating an instance of a class which implements a given interface, we can easily replace this. I did not want to focus on that right now so just used something out of box. Should be done away with as we get closer to getting done. > > Initial code structure looks good package looks good. Agree with I* > classes. We should bring all model classes here. > > I did see some formatting issues. I think Kanak has a eclipse style xml > that fixes the formatting issues. Not a big issue. [Sandeep] I had imported the eclipse style but may have missed out on invoking formatter on each class I touched, will pay attention next time I touch a class. > > To setup eclipse, we need to first run mvn install -DskipTests. > > What are the next steps. If I understand correctly we will move model and > listener api's. I think we should change the listener interfaces and create > event classes. Also we have overloaded INIT and FINALIZE callback that has > caused some issues. [Sandeep] Next steps as I see them (1) identify what is our model, either you (Kishore) or Kanak should edit/update the package-info.java and make sure the bullet list is completed to identify what is our model. (2) clean up the references to classes in core from the model classes (3) move model classes into api, remove the interfaces for model in api, make sure we have configuration objects correctly done for model creation and configuration builders. (4) we need to figure how Json serialization/deserialization is removed from API (5) start on APIs like HelixAdministrator and as we define the APIs we focus on exceptions (6) figure what else needs to move to API like listeners etc need to be moved into api > > Do we have a JIRA, we can add things we want to fix as subTasks and we > revisit later. > > thanks, > Kishore G > > > On Sat, Mar 1, 2014 at 4:53 PM, Sandeep Nayak <[email protected]> wrote: > >> Hey guys, >> >> I checked in some work into the branch >> https://github.com/kanakb/helix. Here is a summary >> >> (1) I have ZNRecord and ZNRecordDelta are kept as is in api. I think >> we need to revisit them and rework them >> >> (2) I have all ids retained with the json deserialization info. >> Removing that would have been a lot of rework and I was focussed on >> moving files into API this first go around. We need to clean up the >> @Json annotations >> >> (3) I have a model package but I decided to hold off on moving all >> models into that package because there were some internal >> dependencies, so I opted to use interfaces for the time being until we >> untangle some of the dependencies of model on core at which point we >> can drop the interfaces and move the model into api cleanly. So bear >> with those IInterface naming conventions, its an eyesore for a reason. >> >> (4) Please note the builder pattern I introduced, there are abstract >> builders in API and the implementations are in core. I decided to >> avoid writing plumbing and get something quick going so leveraged >> apache commons discovery. You can see the pattern in the builder >> packages in api. >> >> (5) API has third party dependencies on google-guava, >> apache-commons-discovery and jackson. We need to bring this down to a >> very minimum as we work through the API cleanup >> >> (6) I added a SerializableZNRecord in org.apache.helix in core because >> I did not want serializer references in the api package. It felt like >> an implementation detail >> >> (7) I added a AbstractRebalancerConfig in >> org.apache.helix.controller.rebalancer.config package in core. Reason >> was the serializer references again. >> >> (8) Most apis use <T extends interface> pattern so that we dont have >> to cast objects in the core project. >> >> (9) Note the ids classes are final deliberately. I would like to keep >> the constructors private but they are accessed in some places so had >> to keep them open. >> >> (10) I moved most of the enums into api. >> >> (11) StateModelDefId was referencing an internal class for a constant >> "SchedulerTaskQueue", I moved that into api and now everywhere the >> core class was referenced I reference the constant id value. >> >> I think I got everything, there is still a lot to be done but I did >> not want to keep going off the deep end and wanted to give everyone a >> chance to review, so we can rework as necessary. >> >> Apologies if I have missed out on formatting in some places, I tried >> to do a thorough job but I am sure I have fallen short in places. It >> has been quite some time since I worked on maven builds so let me know >> if I missed anything in the maven builds. >> >> Thanks, >> >> Sandeep >>
