-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15605/#review29135
-----------------------------------------------------------



http://svn.apache.org/repos/asf/oozie/trunk/core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java
<https://reviews.apache.org/r/15605/#comment56264>

    Currently, if the user is using HA and wants to update all sharelib (and 
tell all Oozie servers), the CLI doesn't do that.  This means that the user 
will have to either call the CLI against each server or use the REST API 
directly.  Also, if the ALL_SERVER is specified, then this server will make a 
call to itself in order to update its sharelib, which is inefficient.  
    
    I recommend you restructure this if-else part of the code like this:
    
    if (ALL_SERVER is specified && ALL_SERVER == false)
    {
        // update this server's sharelib
    }
    else {
        Map serverList = jc.getServerUrls()
        for (Server server : serverList) {
            if (server != me) {
                // do update sharelib call to 'server'
            } else {
                // update this server's sharelib
            }
        }
    }
    
    To check if a server is this server, you can use 
System.getProperty(ZKUtils.OOZIE_INSTANCE_ID) to get the id of this server and 
check it against the serverList.
    
    It would also be good if it would return the status of each server that it 
tried to contact:
    e.g.
    hostA:11000 = Successful
    hostB:11000 = error blah blah



http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/servlet/TestAdminServlet.java
<https://reviews.apache.org/r/15605/#comment56275>

    It would be good to add a similar test for HA.  You should be able to do 
something similar to what I did in 
TestZKXLogStreamingService#testStreamingWithMultipleOozieServers().  
    
    In a little more detail:
    1) You'd have to create a new Test class so it can subclass ZKXTestCase 
(this starts a ZooKeeper server and does some other things)
    2) You can then create a dummy servlet that simply returns the appropriate 
response that the AdminServlet would be expecting (and that you can verify was 
called properly).  My test did this with DummyLogStreamingServlet
    3) Then you can create a DummyZKOozie object whose address points to your 
dummy servlet.  Oozie's ZK code will see this as another Oozie server, which 
you have pointing to your dummy servlet.
    
    Let me know if you have any questions on this.  You can also create a 
separate JIRA and I can write this test after your patch goes in if you prefer.


- Robert Kanter


On Nov. 15, 2013, 11:12 p.m., Purshotam Shah wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15605/
> -----------------------------------------------------------
> 
> (Updated Nov. 15, 2013, 11:12 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1519
>     https://issues.apache.org/jira/browse/OOZIE-1519
> 
> 
> Repository: oozie
> 
> 
> Description
> -------
> 
> Also include fixes of   (OOZIE-1611) Oozie CLI and Rest command to list 
> supported sharelib
> 
> 
> Diffs
> -----
> 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/client/src/main/java/org/apache/oozie/cli/OozieCLI.java
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/client/src/main/java/org/apache/oozie/client/OozieClient.java
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/client/src/main/java/org/apache/oozie/client/rest/JsonTags.java
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/client/src/main/java/org/apache/oozie/client/rest/RestConstants.java
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/core/src/main/java/org/apache/oozie/service/ShareLibService.java
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/core/src/main/java/org/apache/oozie/servlet/V1AdminServlet.java
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/client/TestOozieCLI.java
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/servlet/TestAdminServlet.java
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/docs/src/site/twiki/DG_CommandLineTool.twiki
>  1542384 
>   
> http://svn.apache.org/repos/asf/oozie/trunk/docs/src/site/twiki/WebServicesAPI.twiki
>  1542384 
> 
> Diff: https://reviews.apache.org/r/15605/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Reply via email to