mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 65246: Added download button for master and agent logs in Web UI.
Date Tue, 27 Feb 2018 23:06:35 GMT

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


Fix it, then Ship it!




Thanks Armand! Looks pretty good now, just a few comments below to get through then I should
be able to get this committed for you.


src/webui/master/static/js/controllers.js
Lines 562 (patched)
<https://reviews.apache.org/r/65246/#comment278493>

    



src/webui/master/static/js/controllers.js
Line 565 (original), 562-573 (patched)
<https://reviews.apache.org/r/65246/#comment278494>

    



src/webui/master/static/js/controllers.js
Line 565 (original), 562-573 (patched)
<https://reviews.apache.org/r/65246/#comment278495>

    



src/webui/master/static/js/controllers.js
Lines 562 (patched)
<https://reviews.apache.org/r/65246/#comment278496>

    Why did you need the additional query into the /flags here but not in agent? These fields
should still be in $scope.state, no?



src/webui/master/static/js/controllers.js
Line 565 (original), 564-568 (patched)
<https://reviews.apache.org/r/65246/#comment278483>

    Ditto here.



src/webui/master/static/js/controllers.js
Lines 571-572 (patched)
<https://reviews.apache.org/r/65246/#comment278491>

    Why the `$('#alert').show()` here instead of the `$dialog.messageBox(...).open()` as was
done before? I'm not familiar with the difference, should the UI use a consistent approach?



src/webui/master/static/js/controllers.js
Lines 696-702 (patched)
<https://reviews.apache.org/r/65246/#comment278482>

    Logging enabled is a little misleading of a name here since we're still logging when these
conditions are not held. How about something like `log_file_attached`?
    
    Can we avoid the if condition?
    
    ```
    // The agent attaches a "/slave/log" file when either
    // of these flags are set.
    $scope.agent.log_file_attached = $scope.state.external_log_file || $scope.state.log_dir;
    ```


- Benjamin Mahler


On Feb. 27, 2018, 3:31 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2018, 3:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds a new button to download the logs in addition to the ability to
> view them. Does not display the buttons if the master or agent does not
> have an external log file or a log directory. Displays the correct logs,
> i.e. if you are on a non-leading master you will get the logs of that
> master and not the leading master.
> 
> Regarding the architectural choices, `HomeCtrl` is now similar to the
> other controllers with an update function called when the state changes.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe

> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/5/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1'
--quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2'
--quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1'
--log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from
both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message