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 62047: Allowed look up latest executor directory by virtual path.
Date Fri, 08 Sep 2017 02:33:50 GMT

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



Before I can commit this we need some tests, there is a bug below too in that there was no
leading slash.


src/slave/paths.cpp
Lines 310-317 (patched)
<https://reviews.apache.org/r/62047/#comment261125>

    This does not produce a leading slash.



src/slave/slave.cpp
Line 5568 (original), 5568-5571 (patched)
<https://reviews.apache.org/r/62047/#comment261127>

    Can you file a ticket to remove the real paths post 2.0 and link to it in your comments?



src/slave/slave.cpp
Lines 5578 (patched)
<https://reviews.apache.org/r/62047/#comment261135>

    virtualLatestPath here and elsewhere



src/slave/slave.cpp
Line 5577 (original), 5584-5585 (patched)
<https://reviews.apache.org/r/62047/#comment261133>

    Since we don't need to chain them, can you do:
    
    ```
    garbageCollect(path)
      .then(defer(self(), [=]() {
        detachFile(latestPath);
        detachFile(virtualLatestPath);
      });
    ```



src/slave/slave.cpp
Lines 7287-7290 (original), 7295-7298 (patched)
<https://reviews.apache.org/r/62047/#comment261137>

    I think we could use some more spelling out of what's going on there, to make it easier
on the reader (I myself was puzzled at what was going on with the original two attach calls):
    
    ```
    // We expose the executor's sandbox in the /files endpoints
    // via the following paths:
    // 
    //  (1) /agent_workdir/frameworks/FID/executors/EID/runs/CID
    //  (2) /agent_workdir/frameworks/FID/executors/EID/runs/latest
    //  (3) /frameworks/FID/executors/EID/runs/latest
    //
    // Originally we just exposed the real path (1) and later
    // exposed the 'latest' symlink (2) since it's not easy for
    // users to know the run's container ID. We deprecated
    // (1) and (2) by exposing a virtual path (3) since we do not
    // want to expose the agent's work directory and it's not
    // something users care about in this context.
    //
    // TODO(zhitao): Remove (1) and (2) per MESOS-XXXX once we
    // pass 2.0. They remain now for backwards compatibility.
    ```
    
    You can put my name in the TODO if you like.



src/slave/slave.cpp
Lines 7470-7472 (original), 7491-7493 (patched)
<https://reviews.apache.org/r/62047/#comment261126>

    This needs updating as well to match the other comment, re: real vs virtual instead of
absolute vs relative.


- Benjamin Mahler


On Sept. 7, 2017, 9:18 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62047/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2017, 9:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jason Lai.
> 
> 
> Bugs: MESOS-7899
>     https://issues.apache.org/jira/browse/MESOS-7899
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch added support to attach a virtual path
> `/frameworks/{framework-id}/executors/{executor-id}/latest` to `Files` class
> in agent, which can be used to navigate an executor's latest sandbox without
> needing to know `work_dir` and `slave_id`.
> 
> 
> Diffs
> -----
> 
>   src/slave/paths.hpp 51b481fc0870f1e95448f85ee2fd485fceea1919 
>   src/slave/paths.cpp 1a6943f03493079ab291616d3e36e0fd8809cf33 
>   src/slave/slave.cpp df920ec07cd59c7ba6baccfc1c20ed3809f187d6 
> 
> 
> Diff: https://reviews.apache.org/r/62047/diff/2/
> 
> 
> Testing
> -------
> 
> Ran a master/agent pair, launch a task with `mesos-execute` and use `files/browse` endpoint
to browse the virtual path.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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