Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-17 Thread Jie Yu 

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


Ship it!




Ship It!

- Jie Yu


On Feb. 16, 2016, 11:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> ---
> 
> (Updated Feb. 16, 2016, 11:36 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3193
> https://issues.apache.org/jira/browse/MESOS-3193
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
>   src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
>   src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> ---
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-16 Thread Jojy Varghese 

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

(Updated Feb. 16, 2016, 11:36 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Review addressed.


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


Repository: mesos


Description
---

Added implementation for simple image discovery for Appc images.

TODO: Add tests.


Diffs (updated)
-

  src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
  src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
  src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
  src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 

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


Testing
---

Tested with local http(s) server.


Thanks,

Jojy Varghese

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-13 Thread Shuai Lin 

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



I think this review request should be linked to 
https://issues.apache.org/jira/browse/MESOS-3193 .

- Shuai Lin


On Feb. 13, 2016, 1:43 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> ---
> 
> (Updated Feb. 13, 2016, 1:43 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
>   src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
>   src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> ---
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-13 Thread Jojy Varghese 


> On Feb. 13, 2016, 1:58 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp, line 89
> > 
> >
> > No need for 'doFetchImage' since only this function is using it. Can 
> > you just inline it here?

The idea was that this method will remain common for all discovery types.


> On Feb. 13, 2016, 1:58 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp, line 93
> > 
> >
> > No need for 'const' here as it'll be file local method.

The idea was that all discovery related methods like this would be class 
methods because the fetcher is after all appc discovery spec implementation.


> On Feb. 13, 2016, 1:58 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp, lines 207-212
> > 
> >
> > Hum, do you need to mkdir? will untar create it?

No when i tested with rkt image (coreos/etcd2), after decompressin when i untar 
the tar file, i will untar without creating a directory.


> On Feb. 13, 2016, 1:58 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp, line 214
> > 
> >
> > Do you need to remove aciBundle?
> > 
> > ```
> > .then([]() -> Future {
> >   return os::rm(...);
> > });
> > ```

I could but I thought we agreed that this is a temp directory which will be 
deleted by the caller. The caller would do 'ls' in this directory and pick only 
the directories.


- Jojy


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


On Feb. 13, 2016, 1:43 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> ---
> 
> (Updated Feb. 13, 2016, 1:43 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
>   src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
>   src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> ---
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-12 Thread Jojy Varghese 

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

(Updated Feb. 12, 2016, 9:46 p.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


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


Repository: mesos


Description
---

Added implementation for simple image discovery for Appc images.

TODO: Add tests.


Diffs (updated)
-

  src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
  src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
  src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
  src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 

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


Testing
---

Tested with local http(s) server.


Thanks,

Jojy Varghese

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-12 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [41958, 41959, 43546, 43336]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 12, 2016, 9:46 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> ---
> 
> (Updated Feb. 12, 2016, 9:46 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
>   src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
>   src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> ---
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-12 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [41958, 41959, 43546, 43336]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 13, 2016, 1:43 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> ---
> 
> (Updated Feb. 13, 2016, 1:43 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
>   src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
>   src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> ---
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-12 Thread Jojy Varghese 

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

(Updated Feb. 13, 2016, 1:43 a.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Added implementation for simple image discovery for Appc images.

TODO: Add tests.


Diffs (updated)
-

  src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
  src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
  src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
  src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 

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


Testing
---

Tested with local http(s) server.


Thanks,

Jojy Varghese

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-12 Thread Jie Yu 

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




src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 69)


This can be a file local method, right? No need to declare it in the header.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 41)


We try to avoid global non-POD variables. Can you use char array instead:
```
static const char EXT[] = "aci";
...
```



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 89)


No need for 'doFetchImage' since only this function is using it. Can you 
just inline it here?



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 93)


No need for 'const' here as it'll be file local method.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 118 - 123)


Please use strings::format here



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 127)


`s/imageUrl/_url`

s/getFetchUri/getUri/



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 130)


s/parseURL/parse/



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 137)


s/fetchURL/url/



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 148 - 152)


Move this check up.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 185)


I would

s/imageArchiveFile/aciBundle



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 189)


Let's do `[=]() ...`



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 191)


I would

`s/tmpImageName/_aciBundle/`



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 192)


insert an empty line above



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 201)


Can you be consistent and specify the return value here? Also use [=] for 
capture.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 204)


Ditto on [=]



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 206)


```
const string imagePath = path::join(...);

Try mkdir = os::mkdir(imagePath);
if (mkdir.isError()) {
  ...
}

return ...



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 207 - 212)


Hum, do you need to mkdir? will untar create it?



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 214)


Do you need to remove aciBundle?

```
.then([]() -> Future {
  return os::rm(...);
});
```


- Jie Yu


On Feb. 13, 2016, 1:43 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> ---
> 
> (Updated Feb. 13, 2016, 1:43 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9ab84c0898b3adce6063cc50b04ee74cf1471609 
>   src/Makefile.am 5813ab2c33a7de6b612064e894e5f15b5a474e2b 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 20232d645175d0d574c6d896188435277619010d 
>   src/slave/flags.cpp 14ad4dcc0dfb1d7745e58e11e8f66386288395d7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> ---
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-11 Thread Mesos ReviewBot 

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



Bad patch!

Reviews applied: [41958, 41959, 42156, 42157, 43127, 43198]

Failed command: ./support/apply-review.sh -n -r 43198

Error:
2016-02-11 19:06:40 URL:https://reviews.apache.org/r/43198/diff/raw/ 
[4884/4884] -> "43198.patch" [1]
src/appc/spec.cpp:146:  Redundant blank line at the end of a code block should 
be deleted.  [whitespace/blank_line] [3]
Total errors found: 1
Checking 2 files

Full log: https://builds.apache.org/job/mesos-reviewbot/11393/console

- Mesos ReviewBot


On Feb. 11, 2016, 5:52 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> ---
> 
> (Updated Feb. 11, 2016, 5:52 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4a2954498efa48a4eb43f82827ff1d6f5f65d389 
>   src/Makefile.am 22f51319a1c06da02cac5822d42315e3027cf500 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
>   src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> ---
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-11 Thread Jie Yu 

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



First pass. Will do a more detailed path tomorrow. Thanks Jojy!


src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 30 - 36)


Why do you need this? Can you just include  ?



src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 55 - 56)


Swap the order of these two parameters. We usually put flags first.

Also, please use `const Shared& fetcher`. (You can share 
'Shared' pointers).



src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 68 - 70)


Please remove the 'const' in the end. We try to avoid doing that unless 
it's necessary. Especially, in this case, 'fetch' does not absolutely indicate 
that it's const function (depending on the internal impl.)



src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 73 - 74)


Please fix the indentation for parameters.



src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 74)


NO need for mesos:: namespace prefix since you're in mesos namespace 
already.



src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (lines 76 - 86)


Ditto on removing const in the end.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 29)


Add a new line above.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 53 - 54)


Why use leading underscore for parameters here?



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 83)


No need to create this temp variable. Compiler will do it anyway. User 
appc.name() in all other places sounds pretty clean to me.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 88)


s/discoveryUrl/url/



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 95)


I would just inline this helper here. We try to avoid unnecessary helpers 
because it hurts readability (readers have to jump around in the code).



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 148 - 189)


Hum, I find this unncessary and overly complicated. Can you just assume 
each time we call 'fetch', the 'directory' is an empty temp directory.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 177)


This is not necessary, the caller will cleanup the 'directory'.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 508)


I would split fetch and untar into two steps. At the top level, I would 
like to see something like:

```
return fetch(...)
  .then(lambda::bind(, ...))
  .then(lambda::bind(, ...))
  .onAny(lambda::bind(, ...))
```



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 523 - 527)


Hum, can you move this to the lambda in onAny?


- Jie Yu


On Feb. 11, 2016, 5:52 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> ---
> 
> (Updated Feb. 11, 2016, 5:52 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4a2954498efa48a4eb43f82827ff1d6f5f65d389 
>   src/Makefile.am 22f51319a1c06da02cac5822d42315e3027cf500 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
>   src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> ---
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-11 Thread Jojy Varghese 

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

(Updated Feb. 11, 2016, 5:52 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added implementation for simple image discovery for Appc images.

TODO: Add tests.


Diffs (updated)
-

  src/CMakeLists.txt 4a2954498efa48a4eb43f82827ff1d6f5f65d389 
  src/Makefile.am 22f51319a1c06da02cac5822d42315e3027cf500 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
  src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
  src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 

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


Testing
---

Tested with local http(s) server.


Thanks,

Jojy Varghese

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-11 Thread Jojy Varghese 

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

(Updated Feb. 11, 2016, 5:28 p.m.)


Review request for mesos and Jie Yu.


Changes
---

addressed review.


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


Repository: mesos


Description
---

Added implementation for simple image discovery for Appc images.

TODO: Add tests.


Diffs (updated)
-

  src/CMakeLists.txt 4a2954498efa48a4eb43f82827ff1d6f5f65d389 
  src/Makefile.am 22f51319a1c06da02cac5822d42315e3027cf500 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
  src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
  src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 

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


Testing
---

Tested with local http(s) server.


Thanks,

Jojy Varghese

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-09 Thread Jie Yu 

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



This is a partial review. I scanned the entire patch and realized that the 
semantics of Fetcher is wiered to me.

IIUC, the Fetcher::fetch interface takes an 'Image::Appc' and a 'directory' and 
it'll download all needed layers into 'directory'.

Now, I am thinking about what will the appc::Store::get() be look like? It'll 
have to call 'Fetcher::fetcher' no matter if the name+labels is found or not in 
the cache because even if it's found in the cache, some layers might still be 
missing due to potential cache eviction of layers. Then, it sounds like 
Fetcher::fetch is doing what Store is supposed to be doing.

I am wondering if we should just do the actual fetching in Fetcher and put the 
rest of the logics in appc::Store. In other words, Fetcher::fetch will just 
download a single layer associated with 'Image::Appc', untar it, validate it 
(optional). The Store will be responsible for reading its dependencies and call 
Fetcher::fetch again for those dependencies. In that way, Cache will be 
internal to Store and you don't have to do explicit synchronization.

Based on that, appc::Fetcher can actually be a plugin under uri::Fetcher.

Happy to chat more if you have any question. Thanks Jojy!


src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 24)


This should be
```
#include "slave/flags.hpp"
```



src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 48)


I think we should inject uri::Fetcher here.
```
static Try create(
const Flags& flags,
const Cache& cache,
const Shared& fetcher)
```



src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp (line 67)


const Owned& does not make sense. Please use:
```
Fetcher(Owned process)
```



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 57 - 59)


No need for the 'create' function here. validations should be done in 
Fetcher::create and Fetcher::create should be able to call the constructor 
directly (making the contructor public is OK since it's in a cpp file).



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 59)


Why Option here? Please just use `Flags` here.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 65 - 66)


Can you swap the order of these two parameters?



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 67)


Owned does not sound right since there will be multiple 
entities in the agent sharing the same uri::Fetche in the future.

There're two options here:
1) make uri::Fetcher copyable
2) use Shared

I think 2) is easier currently (you need to mark uri::Fetcher::fetch as a 
const function).



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 87 - 88)


Ditto on swapping the order of these two.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 127 - 129)


Why we need this? 'flags.appc_simple_discovery_uri_prefix' already has a 
default.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 127 - 141)


As I mentioned earlier, this should be moved to Fetcher::create (and kill 
FetcherProcess::create).



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 137 - 141)


OK, this should be injected from top level.



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 171 - 176)


I'd prefer wrapping comments in 70 char width (for better readability).



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (lines 181 - 183)


This looks wierd to me. do simple discovery on Error? I think cache.find 
should return an Option so that the logic here makes more sense (if not found 
in the cache, do simple discovery).



src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp (line 187)


I don't see a reason why spec::getDependencies needs to return an Future. 
It's just reading a file, right? Returning a Try should be sufficient.

Also, I don't like 'getDependencies'. Let's just

Re: Review Request 43336: Introduced Appc image fetcher.

2016-02-08 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [41958, 41959, 42156, 42157, 43127, 43198, 43336]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 8, 2016, 8:30 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43336/
> ---
> 
> (Updated Feb. 8, 2016, 8:30 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for simple image discovery for Appc images.
> 
> TODO: Add tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4a2954498efa48a4eb43f82827ff1d6f5f65d389 
>   src/Makefile.am 22f51319a1c06da02cac5822d42315e3027cf500 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
>   src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
>   src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 
> 
> Diff: https://reviews.apache.org/r/43336/diff/
> 
> 
> Testing
> ---
> 
> Tested with local http(s) server.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 43336: Introduced Appc image fetcher.

2016-02-08 Thread Jojy Varghese 

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added implementation for simple image discovery for Appc images.

TODO: Add tests.


Diffs
-

  src/CMakeLists.txt 4a2954498efa48a4eb43f82827ff1d6f5f65d389 
  src/Makefile.am 22f51319a1c06da02cac5822d42315e3027cf500 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
  src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
  src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 

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


Testing
---

Tested with local http(s) server.


Thanks,

Jojy Varghese