----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35379/#review108278 -----------------------------------------------------------
Like that the logic is lot more concise than the previous versions and you have made good use of changes in OOZIE-2187. But would like optimization to avoid cost of serializing GlobalSectionData for all workflows which do not even have sub-workflows and also the deserialization of GlobalSectionData done for every action again for all workflows which are unnecessary. Suggested changes below which will serialize global section into jobconf only when there is sub-workflow with propagate-configuration. Also deserialization of GlobalSectionData from jobconf is done only once in the beginning. core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java (line 431) <https://reviews.apache.org/r/35379/#comment167645> GlobalSectionData gData = jobConf.get(OOZIE_GLOBAL) == null ? null : getGlobalFromString(jobConf.get(OOZIE_GLOBAL)) boolean serializedGlobalConf = false; core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java (lines 474 - 477) <https://reviews.apache.org/r/35379/#comment167632> Instead of propagateGlobalConf = true; can we do if (!serializedGlobalConf && elem.getName().equals(SubWorkflowActionExecutor.ACTION_TYPE) && elem.getChild(("propagate-configuration"), ns) != null) { serializedGlobalConf = true; jobConf.set(OOZIE_GLOBAL, getGlobalString(gData)); } eActionConf = elem; handleDefaultsAndGlobal(gData, configDefault, elem); here instead of in eNode.getName().equals(GLOBAL) section and remove if (!propagateGlobalConf) { jobConf.unset(OOZIE_GLOBAL); } serializedGlobalConf is to avoid serializing multiple times if there is more than one sub-workflow which is the case most of the time. core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java (lines 503 - 507) <https://reviews.apache.org/r/35379/#comment167648> if(jobConf.get(OOZIE_GLOBAL) != null) { handleDefaultsAndGlobal(gData, null, eNode); } gData = parseGlobalSection(ns, eNode); gData = parseGlobalSection(ns, eNode); at the beginning before if block needs to be removed. Parse gData only once after the if block. core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java (line 551) <https://reviews.apache.org/r/35379/#comment167630> Can you compress before base64 encoding? It is compressed before storing to database. But in some recent issues with hcat we found that conf compresses lot better before base64 encoding than after it. So compressing here should also be good. Deflater def = new Deflater(); oos = new DataOutputStream(new DeflaterOutputStream(baos, def)); core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java (lines 643 - 646) <https://reviews.apache.org/r/35379/#comment167629> Make it an empty constructor. null is the default value with the variable declaration. If you want it to be explicitly null, assign it to null in the variable declaration itself core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java (line 709) <https://reviews.apache.org/r/35379/#comment167631> Shouldn't foo3 be actionconf here also? - Rohini Palaniswamy On July 21, 2015, 10:59 a.m., Jaydeep Vishwakarma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35379/ > ----------------------------------------------------------- > > (Updated July 21, 2015, 10:59 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2030 > https://issues.apache.org/jira/browse/OOZIE-2030 > > > Repository: oozie-git > > > Description > ------- > > Forwarding global conf from workflow to action when action is subworkflow, > subworkflow storing it to property and forwarding as property. > LiteworkflowAppParser handling all condition and ensuring child level pass > of global conf. > > > Diffs > ----- > > > core/src/main/java/org/apache/oozie/action/oozie/SubWorkflowActionExecutor.java > 854d621 > > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowAppParser.java > d3a6523 > > core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java > 9ab897a > > Diff: https://reviews.apache.org/r/35379/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Jaydeep Vishwakarma > >
