See responses inline On Sat, Mar 1, 2014 at 5:54 PM, kishore g <[email protected]> wrote: > Few comments. > > Partition & State should be in model package instead of config. Not sure > where should we have Scope class.
[Sandeep] I think I moved them from the config package as was in core but now that I look at them it does make sense to move them to model. What is Scope, where does it get tied to? Lets chat online. > > HelixProperty should not contain batchMessage mode, bucket size etc. This > existed before and it is having too many things. The main purpose of this > class is to represent a HelixRecord. [Sandeep] Again moved it as is, did not alter the class. I want to first focus on what goes where and then focus on what each of the classes should contain. > > Here is the high level overview of how data is organized in zookeeper and > how it is represented in the code. > > Every Zookeeper path that contains a set of child nodes is a table. We > every ZNode represents a HelixProperty. Each HelixProperty consists of two > things > > * Key: this is can be single key or composite/heirarchical. We use > PropertyKey to represent this. > * Value: This represents the content of the znode. We use ZNRecord to > represent this. > > Most classes like IdealState, ExternalView etc are of HelixProperty type. > > The reason for doing this is, we have only one class (DataAccessor) to > interact with the zookeeper layer. This DataAccessor class has to support > simple CRUD operations. [Sandeep] If ZNRecord is a representation for zookeeper then it should not be in HelixProperty. We need to remove it, so we make it implementation independent. Lets chat online on this too. > > Does this make sense? > > > > > > 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. >> >> 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. >> >> 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. >> >> 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 >>> >> >>
