> On 2012-03-06 15:23:53, Paul Ramirez wrote: > > Looks good most of my comments are a request for some unit tests especially > > where stuff has been refactored and there is new code. Some of the stuff > > wasn't a change as the code was just reformatted.
Hey paul thanks for the comments, also should have noted that i'm gonna use this as like the master patch... will make a visual for me so i can create smaller incremental patches (if possible... lol)... lots of bug fixes and improvements were added to wengine's cas-pge... also cas-pge needs unit tests... currently only has one test which does nothing! lol... also needs cas-cli integration as well, so xml validation actions and the like can be written and possibly actions like workflow emulation mode actions so cas-pge can be run stand-alone but act like it is part of a workflow... this will probably take me the next century to accomplish but i'm gonna start chopping away at it :) > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/PGETaskInstance.java, lines > > 146-149 > > <https://reviews.apache.org/r/4196/diff/1/?file=88545#file88545line146> > > > > Why is this gone? I see the new one that runs all. Mark this with > > deprecated? It's protected so only internally used just wondered. not gone, signature changed to: protected void runPropertyAdder(ConfigFilePropertyAdder propAdder, PgeConfig pgeConfig, PgeMetadata pgeMetadata) { > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/PGETaskInstance.java, lines > > 238-239 > > <https://reviews.apache.org/r/4196/diff/1/?file=88545#file88545line238> > > > > Not really a change just a reformatting. The class lacked formatting... updated it to 3 space formatting > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/PGETaskInstance.java, line > > 282 > > <https://reviews.apache.org/r/4196/diff/1/?file=88545#file88545line282> > > > > no change just reformatting The class lacked formatting... updated it to 3 space formatting > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/PGETaskInstance.java, lines > > 464-465 > > <https://reviews.apache.org/r/4196/diff/1/?file=88545#file88545line464> > > > > Is this one of the keys you were saying is still needed in the patch? > > yup > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/PGETaskInstance.java, lines > > 548-550 > > <https://reviews.apache.org/r/4196/diff/1/?file=88545#file88545line548> > > > > Where has this functionality moved to? Still gotta work this back in... wengine's TaskInstance would update a tasks metadata for you... PgeMetadata existed in wengine as ControlMetadata > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/config/FileBasedPgeConfigBuilder.java, > > line 41 > > <https://reviews.apache.org/r/4196/diff/1/?file=88546#file88546line41> > > > > This appears to be a refactoring make sure the unit tests written for > > the XMLFilePGEConfigBuilder cover the code here. will do > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/config/XmlFilePgeConfigBuilder.java, > > line 69 > > <https://reviews.apache.org/r/4196/diff/1/?file=88551#file88551line69> > > > > Lots of changes needs unit tests will do! > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/metadata/PGETaskMetKeys.java, > > line 3 > > <https://reviews.apache.org/r/4196/diff/1/?file=88552#file88552line3> > > > > yay constants. acknowledged > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/metadata/PgeMetadata.java, > > line 241 > > <https://reviews.apache.org/r/4196/diff/1/?file=88553#file88553line241> > > > > Yay unused code removed. acknowledged > On 2012-03-06 15:23:53, Paul Ramirez wrote: > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/staging/FileManagerFileStager.java, > > line 1 > > <https://reviews.apache.org/r/4196/diff/1/?file=88554#file88554line1> > > > > Needs unit tests. Also make sure there is coverage into FileStager. will do! - brian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4196/#review5642 ----------------------------------------------------------- On 2012-03-06 08:20:36, brian Foster wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4196/ > ----------------------------------------------------------- > > (Updated 2012-03-06 08:20:36) > > > Review request for oodt, Chris Mattmann and Paul Ramirez. > > > Summary > ------- > > This includes the changes from wengine-branch cas-pge... still needs to add > backwards compatibility for MetKeys (i.e. PCS_* etc) > > > Diffs > ----- > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/PGETaskInstance.java > 1297147 > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/config/FileBasedPgeConfigBuilder.java > PRE-CREATION > trunk/pge/src/main/java/org/apache/oodt/cas/pge/config/FileStagingInfo.java > PRE-CREATION > trunk/pge/src/main/java/org/apache/oodt/cas/pge/config/PgeConfig.java > 1297147 > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/config/PgeConfigBuilder.java > 1297147 > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/config/PgeConfigMetKeys.java > 1297147 > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/config/XmlFilePgeConfigBuilder.java > 1297147 > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/metadata/PGETaskMetKeys.java > PRE-CREATION > trunk/pge/src/main/java/org/apache/oodt/cas/pge/metadata/PgeMetadata.java > 1297147 > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/staging/FileManagerFileStager.java > PRE-CREATION > trunk/pge/src/main/java/org/apache/oodt/cas/pge/staging/FileStager.java > PRE-CREATION > > trunk/pge/src/main/java/org/apache/oodt/cas/pge/writers/PcsMetFileWriter.java > 1297147 > > Diff: https://reviews.apache.org/r/4196/diff > > > Testing > ------- > > > Thanks, > > brian > >
