HUE-7932 [oozie] Clicking on workflow with several decision nodes hangs Hue

Review Request #12444 — Created Jan. 31, 2018 and submitted

roohi
hue
HUE-7774-Autocomplete
HUE-7932
hue
enricoberti, jgauthier, johan, ranade, romain, weixia, yingc
commit 16e2f641bb715ec12fe44e44ab3e588da5375aaa
Author: Roohi <roohisyeda@cloudera.com>
Date:   Wed Jan 31 23:52:43 2018 -0800

    HUE-7932 [oozie] Clicking on workflow with several decision nodes hangs Hue

:100644 100644 aa5c41a976... e72e8c4466... M	apps/oozie/src/oozie/models2.py


  • 4
  • 0
  • 8
  • 3
  • 15
Description From Last Updated
Start of workflow with more than 25 levels roohi roohi
End of workflow with more than 25 levels roohi roohi
Start of workflow with 24 levels roohi roohi
End of workflow with 24 levels roohi roohi
romain
  1. We alreayd have a set of tests on this I think?
    But could we add one with > 25 levels?

    1. Sure. Can I add the same one as the one with Escalation issue? as a file?

  2. apps/oozie/src/oozie/models2.py (Diff revision 1)
     
     

    Don't we have the same constant limit similarly in the editor somewhere?

    1. Are you talking about this one https://github.com/cloudera/hue/blob/d78f992fa320fcbcafe2090457ba95e133611e6a/apps/oozie/src/oozie/views/dashboard.py#L359 ?
      This is for the total number of nodes, after the Graph is created

  3. 
      
roohi
roohi
  1. 
      
  2. apps/oozie/src/oozie/models2_tests.py (Diff revision 2)
     
     

    Start of workflow with more than 25 levels

  3. apps/oozie/src/oozie/models2_tests.py (Diff revision 2)
     
     

    End of workflow with more than 25 levels

  4. apps/oozie/src/oozie/models2_tests.py (Diff revision 2)
     
     

    Start of workflow with 24 levels

  5. apps/oozie/src/oozie/models2_tests.py (Diff revision 2)
     
     

    End of workflow with 24 levels

  6. 
      
romain
  1. 
      
  2. apps/oozie/src/oozie/models2.py (Diff revision 2)
     
     

    Any way to use the same limit variable as in the editor?

    1. No, the reason for HUE hanging is not related to total number of nodes but with how deep the graph is from start to end. It will work fine if there are only 2 levels with say a decision/fork having many (more than 25) switch case nodes.

  3. apps/oozie/src/oozie/models2.py (Diff revision 2)
     
     

    Do we need a new method while get_hierarchy_from_adj_list_helper() could count its level and raise an exception if we reach the limit?

    1. I initially thought about it. But I didn't want to introduce any regression and keep it in a new method, as it might involve more testing and delay the patch delivery. What do you think about it?

  4. 
      
romain
  1. 
      
  2. apps/oozie/src/oozie/models2.py (Diff revision 2)
     
     

    I see, difference from total number of node.

    So could we move the variable out below line #56 and rename it in a better way, e.g.

    WORKFLOW_DEPTH_LIMIT = 25

    ?

  3. apps/oozie/src/oozie/models2.py (Diff revision 2)
     
     

    Seems safer to add a n=LIMIT parameter to get_hierarchy_from_adj_list_helper and just decrement it on each recursion instead of having a clone of the tree navigation logic?. Also less code to maintain later.

    And then e.g.:

    class WorkflowDepthReached(Exception):
    pass

    if workflow_depth <= 0:
    raise WorkflowDepthReached()

    try:
    ..
    except WorkflowDepthReached:
    pass

    ?

  4. 
      
roohi
romain
  1. 
      
  2. apps/oozie/src/oozie/models2.py (Diff revision 3)
     
     
     

    nit: spaces around -

    and next 2 below too

  3. apps/oozie/src/oozie/models2_tests.py (Diff revision 3)
     
     

    CTRL+replace all the trailing spaces?

  4. 
      
romain
  1. Few more nits, but great overall!

  2. apps/oozie/src/oozie/models2.py (Diff revision 3)
     
     

    Add name and id of worfklow to the message?

  3. apps/oozie/src/oozie/models2.py (Diff revision 3)
     
     
     

    why this except?

    1. This is the faster way of getting out of the recursion or else we still explore other paths (line 736) which might be deeper than this and might again raise exception.

  4. 
      
roohi
romain
  1. Last ones!

  2. apps/oozie/src/oozie/models2.py (Diff revision 4)
     
     

    needed?

  3. apps/oozie/src/oozie/models2.py (Diff revision 4)
     
     
     

    Still not sure why this whole tr/except is needed if we just catch-re-raise?

    1. Got it. I thought you were asking about why we needed raise in line 758. You are right we don't need the whole try/except block itself. Thanks Romain for the suggestions.

  4. 
      
roohi
romain
  1. Ship It!
  2. 
      
roohi
Review request changed

Status: Closed (submitted)

Loading...