----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7801/#review12942 -----------------------------------------------------------
Good start. We can talk on any comment that is not clear. http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27847> in comment, at least write what is key and what is value? http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27848> same http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27849> same http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27853> do we need to add the default server by default? may be no body is using the default server. http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27860> partitionKV could be a list of part_key =part _val. For example, ds=20120912, region=us is ***one** partition. http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27850> Provide another method with same name but with two parameters: HCatURI, actionID. The implementation will call this one after parsing the URI using Ryota's utility http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27857> throwing more specific exception other than generalized exception XException could be better. http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27854> pls give more meaningful name (i.e. serverDB) http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27855> this is not really a partition id. right? More importantly, is it a good idea in this case. In most of the cases, one table could have thousands of partitions. Every key will have extra "table name:. Might need to revisit this. http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27856> This and next line could be merged into one call. PartitionInfo could have another constructor which take actionID as param. This is the common usage. So achieving the same in one call will be better. http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27851> same . another method with hcatURI http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27859> if this is the last item in missing Map, we should remove from hcatInstanceMap. http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27858> wandering throw exception for non-existence would be a good idea. May be just log an warning and skip it. may be return false for non-existence. http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java <https://reviews.apache.org/r/7801/#comment27852> same. another similar method with two params. http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/util/PartitionInfo.java <https://reviews.apache.org/r/7801/#comment27861> class name 'PartitionInfo' looks misnomer. There is nothing related to partition. It store a list of actions and TS. - Mohammad Islam On Oct. 31, 2012, 2:02 a.m., Mona Chitnis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7801/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2012, 2:02 a.m.) > > > Review request for oozie. > > > Description > ------- > > WIP. Implementing the data structures and interfacing methods which are used > by all other services > > > This addresses bug OOZIE-1039. > https://issues.apache.org/jira/browse/OOZIE-1039 > > > Diffs > ----- > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/pom.xml > 1403871 > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/service/PartitionDependencyManagerService.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/main/java/org/apache/oozie/util/PartitionInfo.java > PRE-CREATION > > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/core/src/test/java/org/apache/oozie/service/TestPartitionDependencyManager.java > PRE-CREATION > http://svn.apache.org/repos/asf/oozie/branches/hcat-intre/pom.xml 1403871 > > Diff: https://reviews.apache.org/r/7801/diff/ > > > Testing > ------- > > unit tests in progress > > > Thanks, > > Mona Chitnis > >
