> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > cli/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java, line 305 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485662#file1485662line305> > > > > Also add parseDateString(effectiveTime), to ensure the date is of valid > > format
Added. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, > > line 117 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line117> > > > > We are assuming /tmp exists. Can you use cluster's "temp" location > > instead? Ack. Changed to cluster temp location > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, > > line 139 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line139> > > > > This creates new jars. How about updated jars? We are not using the > > checksum in the dictionary? Modified to check the checksum of any existing jar while checking for the jar entry. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, > > line 147 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line147> > > > > Should we also write the dictionary file in this method itself? Yes. Moved the writing of the file to this method. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, > > line 151 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line151> > > > > How will the invoker of this method interpret null? This method is now modified to just return the map of entries. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, > > line 168 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line168> > > > > Should it be "getMostRecentDictionaryPath"? Method name is a little > > confusing. Actually the comparator is written wrongly. Corrected it. I am comparing it with the earliest Dictionary than recent Dictionary > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/update/UpdateHelper.java, line 238 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485671#file1485671line238> > > > > We are checking for checksum of only workflow.xml and not jars? > > Shouldn't be iterating over the checksums variable? Only worfklow is being tracked here. Jars have changed or not , the update will continue with new coordinator from the effective time but with almost the same entry as the previous entry. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/update/UpdateHelper.java, line 250 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485671#file1485671line250> > > > > Log and throw. Ack. Logged > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/workflow/engine/AbstractWorkflowEngine.java, > > line 119 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485673#file1485673line119> > > > > Why does this need to be part of AbstractWorkflowEngine. Can't it be > > private method? Kept it so that the same method can be implemented falcon workflow engine too when required. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java, > > line 113 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485686#file1485686line113> > > > > Not a good idea to pass variables that get modified by methods. > > > > If you pull writeLibDict into createLibDictionary, you can read it > > back. Better would be for createLibDictionary to return the dictionary and > > take in the final path. Made the changes accordingly to call write method inside createLibDictionary. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java, > > line 123 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485686#file1485686line123> > > > > As mentioned in another comment, may be createLibDictionary should > > write the dictionary file too, rather than doing it as a separate step. > > Then, jars and the dictionary file are created together. Made the changes accordingly > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java, > > line 1909 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485688#file1485688line1909> > > > > Should we also ensure the retry doesn't kick in for these instances? > > Or, is it inherently taken care of? Added ignoreInstances in the next line. Had missed it. Taking care of it by ignoring these instances, where falcon retry doesnt retry on a instance that is ignored. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java, > > line 581 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485693#file1485693line581> > > > > As mentioned above, please make this private method. This need not be > > exposed as a public method. Ack. Removed it from FalconWorkflowEngine > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionWorkflowBuilder.java, > > line 131 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485687#file1485687line131> > > > > IOException does not mean file not found. Modified the error code and throwing out the exception now. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, > > line 76 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line76> > > > > Nit: Better to keep the method name and parameter names of read/write > > methods similar, for readabiltiy sake. renamed write method in par with read method. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, > > line 66 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line66> > > > > EntityLibDictionary does not have a toString() method. You might want > > to verify if the serialization/deserialization is happening correctly. Fixed it. Implemented toString to EntityLibEntry(EntityLibDictionary). > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, > > line 63 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line63> > > > > Should we also set the perms appropriately? Have added the permissions in accordance. > On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote: > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java, > > line 1300 > > <https://reviews.apache.org/r/51424/diff/1/?file=1485688#file1485688line1300> > > > > Cleaner to do the isWfUpdated check inside this method. Ack. moved isWfUpdaated to canUpdateBundle. - sandeep ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51424/#review147551 ----------------------------------------------------------- On Oct. 19, 2016, 3:03 p.m., sandeep samudrala wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51424/ > ----------------------------------------------------------- > > (Updated Oct. 19, 2016, 3:03 p.m.) > > > Review request for Falcon and Pallavi Rao. > > > Repository: falcon-git > > > Description > ------- > > Effective Time in Entity Update > > > Diffs > ----- > > cli/src/main/java/org/apache/falcon/cli/FalconCLI.java 0dd11f6 > cli/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java a8aea52 > client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java > 5d6eff5 > client/src/main/java/org/apache/falcon/client/FalconCLIConstants.java > 04f1599 > client/src/main/java/org/apache/falcon/client/FalconClient.java 8f77fad > common/src/main/java/org/apache/falcon/entity/ClusterHelper.java f89def3 > common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java > PRE-CREATION > common/src/main/java/org/apache/falcon/entity/EntityLibEntry.java > PRE-CREATION > common/src/main/java/org/apache/falcon/entity/EntityUtil.java 8fe316c > common/src/main/java/org/apache/falcon/entity/ProcessHelper.java e563d18 > > common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java > 38fa3ae > common/src/main/java/org/apache/falcon/update/UpdateHelper.java 266319f > > common/src/main/java/org/apache/falcon/workflow/engine/AbstractWorkflowEngine.java > 16a1753 > common/src/test/java/org/apache/falcon/entity/EntityDictionaryUtilTest.java > PRE-CREATION > common/src/test/java/org/apache/falcon/update/UpdateHelperTest.java 826686f > docs/src/site/twiki/falconcli/Touch.twiki afbd848 > docs/src/site/twiki/falconcli/UpdateEntity.twiki 146a60f > oozie/src/main/java/org/apache/falcon/oozie/OozieBundleBuilder.java 5f93cc2 > oozie/src/main/java/org/apache/falcon/oozie/feed/FeedBundleBuilder.java > c758411 > > oozie/src/main/java/org/apache/falcon/oozie/process/HiveProcessWorkflowBuilder.java > 9f9579c > > oozie/src/main/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilder.java > f93a599 > > oozie/src/main/java/org/apache/falcon/oozie/process/PigProcessWorkflowBuilder.java > a1a7c12 > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java > 6661dd5 > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionCoordinatorBuilder.java > 91f4757 > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionWorkflowBuilder.java > 20eeffd > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java > 394600c > > oozie/src/test/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilderTest.java > 05b513e > oozie/src/test/resources/config/process/dumb-hive-process.xml c504074 > oozie/src/test/resources/config/process/hive-process-FSInputFeed.xml > d871377 > oozie/src/test/resources/config/process/hive-process-FSOutputFeed.xml > 23d96c3 > oozie/src/test/resources/config/process/hive-process.xml 4dac8e9 > oozie/src/test/resources/config/process/pig-process-0.1.xml 8d20cee > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java > aefd699 > > prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java > 3bdeb99 > > prism/src/main/java/org/apache/falcon/resource/extensions/ExtensionManager.java > 92b5531 > > prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java > 07334d6 > > scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java > 9ba62a1 > > shell/src/main/java/org/apache/falcon/shell/commands/FalconEntityCommands.java > 35a6f2a > src/build/checkstyle.xml 292a0a3 > unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java 53073f0 > > unit/src/main/java/org/apache/falcon/unit/LocalSchedulableEntityManager.java > 7398c8a > unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java bfc8b08 > unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java 0bc7755 > unit/src/test/resources/process.xml 6854311 > webapp/src/main/java/org/apache/falcon/resource/ConfigSyncService.java > 7b32bd5 > > webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java > 5525207 > webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseyIT.java > 876ada5 > > Diff: https://reviews.apache.org/r/51424/diff/ > > > Testing > ------- > > > Thanks, > > sandeep samudrala > >
