Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Greg Mann 

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

(Updated July 20, 2016, 11:01 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Changes
---

Addressed comments.


Bugs: MESOS-5845
https://issues.apache.org/jira/browse/MESOS-5845


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs (updated)
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

Diff: https://reviews.apache.org/r/50199/diff/


Testing
---

`sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
chain.


Thanks,

Greg Mann

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Greg Mann 


> On July 20, 2016, 10:26 p.m., Jie Yu wrote:
> > src/launcher/fetcher.cpp, lines 512-514
> > 
> >
> > Can we instead use:
> > ```
> > CHECK_SOME(createCacheDirectory(fetcherInfo.get()))
> >   << "...";
> > ```

I changed this to an `EXIT` statement, per your suggestion on the following 
patch.


- Greg


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


On July 20, 2016, 11:01 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 20, 2016, 11:01 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Jie Yu 

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


Fix it, then Ship it!





src/launcher/fetcher.cpp (lines 506 - 508)


Can we instead use:
```
CHECK_SOME(createCacheDirectory(fetcherInfo.get()))
  << "...";
```



src/launcher/fetcher.cpp (line 430)


for such case, we usually do the following to avoid more level of nesting:
```
if (!fetcherInfo.has_cache_directory()) {
  return Nothing();
}

foreach (...)
```



src/launcher/fetcher.cpp (line 436)


s/result/mkdir/



src/launcher/fetcher.cpp (lines 444 - 445)


I'd prefer
```
Try chown = os:chown(
fetcherInfo.user(),
fetcherInfo.cache_directory(),
false);

if (chown.isError()) {
  return chown;
}
```


- Jie Yu


On July 20, 2016, 10:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 20, 2016, 10:07 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Greg Mann 


> On July 20, 2016, 10:06 p.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 441
> > 
> >
> > Can it happen that dir already exists?

Indeed it can! While we were safe with the existing code, I tweaked this call 
to `os::mkdir` to explicitly set `recursive = true`, so that we won't get an 
error if the directory already exists. Thanks Joerg!!


- Greg


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


On July 20, 2016, 10:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 20, 2016, 10:07 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Greg Mann 

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

(Updated July 20, 2016, 10:07 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Changes
---

Addressed comment.


Bugs: MESOS-5845
https://issues.apache.org/jira/browse/MESOS-5845


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs (updated)
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

Diff: https://reviews.apache.org/r/50199/diff/


Testing
---

`sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
chain.


Thanks,

Greg Mann

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Joerg Schad 

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




src/launcher/fetcher.cpp (line 435)


Can it happen that dir already exists?


- Joerg Schad


On July 20, 2016, 7:50 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 20, 2016, 7:50 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Joerg Schad 


> On July 20, 2016, 8:14 a.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 514
> > 
> >
> > I personally would still use this cacheDirectory as parameter to 
> > createCacheDirectory for the following reason: we need to extract it anyhow 
> > and otherwise we do that at two different places. Feel free to drop if you 
> > have a different opinion.
> 
> Greg Mann wrote:
> With the current diff, the only extra work done in `createCacheDirectory` 
> is `if (fetcherInfo.has_cache_directory())`, which seems OK to me. I think 
> that we could definitely refactor the various fetching functions in this file 
> to clean things up, but perhaps we could do that in separate patches.

It is no issue :-), it just felt weird when reading the code that you first 
create the directory and afterwards extract the directory name from the 
protobuf.


- Joerg


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


On July 20, 2016, 7:50 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 20, 2016, 7:50 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Greg Mann 

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

(Updated July 20, 2016, 7:50 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Changes
---

Addressed comments.


Bugs: MESOS-5845
https://issues.apache.org/jira/browse/MESOS-5845


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs (updated)
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

Diff: https://reviews.apache.org/r/50199/diff/


Testing
---

`sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
chain.


Thanks,

Greg Mann

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Greg Mann 


> On July 20, 2016, 8:14 a.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 436
> > 
> >
> > Do we really want to check and fail on this condition? In the previous 
> > iteration you would have skipped creating the directory in that case.
> > Also judging from the code ``  const Option cacheDirectory =
> > fetcherInfo.get().has_cache_directory() ?
> >   Option::some(fetcherInfo.get().cache_directory()) :
> > Option::none();``` below, it seems it is valid that 
> > there is no cache_directory.
> 
> Joerg Schad wrote:
> just seen your comment on the previous patch set. Does it in that case 
> make sense to add a check and change the logic of the above mentioned code?

So actually, I do think that I should remove this check :) sorry for the 
confusion...

I'm inclined to leave the code that you mentioned as-is; while our current 
implementation does guarantee that `FetcherInfo` will always contain a cache 
directory, there's no reason that this _must_ be the case if the cache is being 
bypassed, so I don't think we should enforce it in the fetcher binary itself. 
Since `fetchThroughCache` performs the necessary check when the cache _is_ 
being used, I think the code you quoted is OK. However, in order to make it so 
the fetcher binary does not assume that the cache directory is always included, 
I should remove this CHECK.


> On July 20, 2016, 8:14 a.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 510
> > 
> >
> > Does the name `result` make sense at this point?
> > Maybe something like `cacheDirectory`?

I think this is OK, since `result` doesn't contain the cache directory itself, 
the `Try` is just used to propagate an error. I wonder if an `Option` 
would be better?


> On July 20, 2016, 8:14 a.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 438
> > 
> >
> > This comment should move one line down or? (as you now have the create 
> > logic in the loop as well).

I expanded this comment a bit so that it makes sense (I hope) in its current 
location.


> On July 20, 2016, 8:14 a.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 514
> > 
> >
> > I personally would still use this cacheDirectory as parameter to 
> > createCacheDirectory for the following reason: we need to extract it anyhow 
> > and otherwise we do that at two different places. Feel free to drop if you 
> > have a different opinion.

With the current diff, the only extra work done in `createCacheDirectory` is 
`if (fetcherInfo.has_cache_directory())`, which seems OK to me. I think that we 
could definitely refactor the various fetching functions in this file to clean 
things up, but perhaps we could do that in separate patches.


- Greg


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


On July 20, 2016, 7:50 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 20, 2016, 7:50 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Joerg Schad 


> On July 20, 2016, 8:14 a.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 436
> > 
> >
> > Do we really want to check and fail on this condition? In the previous 
> > iteration you would have skipped creating the directory in that case.
> > Also judging from the code ``  const Option cacheDirectory =
> > fetcherInfo.get().has_cache_directory() ?
> >   Option::some(fetcherInfo.get().cache_directory()) :
> > Option::none();``` below, it seems it is valid that 
> > there is no cache_directory.

just seen your comment on the previous patch set. Does it in that case make 
sense to add a check and change the logic of the above mentioned code?


- Joerg


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


On July 19, 2016, 11:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 19, 2016, 11:10 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-20 Thread Joerg Schad 

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




src/launcher/fetcher.cpp (line 430)


Do we really want to check and fail on this condition? In the previous 
iteration you would have skipped creating the directory in that case.
Also judging from the code ``  const Option cacheDirectory =
fetcherInfo.get().has_cache_directory() ?
  Option::some(fetcherInfo.get().cache_directory()) :
Option::none();``` below, it seems it is valid that there 
is no cache_directory.



src/launcher/fetcher.cpp (line 432)


This comment should move one line down or? (as you now have the create 
logic in the loop as well).



src/launcher/fetcher.cpp (line 441)


task's user? I am not a native speaker but 'as the task user' sounds 
strange to me, feel free to drop if correct.



src/launcher/fetcher.cpp (line 504)


Does the name `result` make sense at this point?
Maybe something like `cacheDirectory`?



src/launcher/fetcher.cpp (line 508)


I personally would still use this cacheDirectory as parameter to 
createCacheDirectory for the following reason: we need to extract it anyhow and 
otherwise we do that at two different places. Feel free to drop if you have a 
different opinion.


- Joerg Schad


On July 19, 2016, 11:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 19, 2016, 11:10 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-19 Thread Greg Mann 


> On July 19, 2016, 8:51 p.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 381
> > 
> >
> > Does it make sense to add a check here to document and check that 
> > precondition?

Yep! :)


> On July 19, 2016, 8:51 p.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 437
> > 
> >
> > Not an issue: just making sure I understood this correctly :-): if 
> > there is no user we do not need to create directory i.e., a global/non 
> > user-specific cache dir is created upfront?

While I do think this was a correct assumption (that the directory will already 
exist if no task user is specified), the `FetcherInfo` passed to the fetcher 
binary should always contain a cache directory if the cache is going to be used 
(see 
https://github.com/apache/mesos/blob/dc4a4ae9b264e66ff891c6b00181d8d501514642/src/launcher/fetcher.cpp#L368-L370),
 so I think it makes more sense to follow the original behavior found here, 
where we call `os::mkdir` whether or not a task user has been supplied.


- Greg


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


On July 19, 2016, 11:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 19, 2016, 11:10 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-19 Thread Greg Mann 

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

(Updated July 19, 2016, 11:10 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Changes
---

Addressed comments.


Bugs: MESOS-5845
https://issues.apache.org/jira/browse/MESOS-5845


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs (updated)
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

Diff: https://reviews.apache.org/r/50199/diff/


Testing
---

`sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
chain.


Thanks,

Greg Mann

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-19 Thread Gilbert Song 

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




src/launcher/fetcher.cpp (line 433)


This var may not be necessary.



src/launcher/fetcher.cpp (lines 442 - 448)


Could we move this logic inside `if (item.action() != 
FetcherInfo::Item::BYPASS_CACHE) {}`?


- Gilbert Song


On July 19, 2016, 1:54 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 19, 2016, 1:54 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-19 Thread Greg Mann 

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

(Updated July 19, 2016, 8:54 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Bugs: MESOS-5845
https://issues.apache.org/jira/browse/MESOS-5845


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

Diff: https://reviews.apache.org/r/50199/diff/


Testing (updated)
---

`sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
chain.


Thanks,

Greg Mann

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-19 Thread Joerg Schad 

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




src/launcher/fetcher.cpp (line 381)


Does it make sense to add a check here to document and check that 
precondition?



src/launcher/fetcher.cpp (line 431)


Not an issue: just making sure I understood this correctly :-): if there is 
no user we do not need to create directory i.e., a global/non user-specific 
cache dir is created upfront?



src/launcher/fetcher.cpp (line 448)


Should we check whether chown was sucessful?



src/launcher/fetcher.cpp (line 508)


Is there a reason to do this outside `createCacheDirectory`? We are already 
passing fetcherInfo...



src/launcher/fetcher.cpp (line 512)


Should we check whether an error was returned?


- Joerg Schad


On July 19, 2016, 8:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 19, 2016, 8:07 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `make check` on CentOS 7 was done at the end of this patch chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 50199: Refactored fetcher cache directory creation.

2016-07-19 Thread Greg Mann 

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

Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Bugs: MESOS-5845
https://issues.apache.org/jira/browse/MESOS-5845


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

Diff: https://reviews.apache.org/r/50199/diff/


Testing
---

`make check` on OSX and CentOS 7 was done at the end of this patch chain.


Thanks,

Greg Mann

Re: Review Request 50199: Refactored fetcher cache directory creation.

2016-07-19 Thread Greg Mann 

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

(Updated July 19, 2016, 8:07 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Bugs: MESOS-5845
https://issues.apache.org/jira/browse/MESOS-5845


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

Diff: https://reviews.apache.org/r/50199/diff/


Testing (updated)
---

`make check` on CentOS 7 was done at the end of this patch chain.


Thanks,

Greg Mann