Re: Review Request 43127: Introduced Appc image cache.

[Jojy Varghese]

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

(Updated Feb. 11, 2016, 4:15 p.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-4439 and MESOS-4575
https://issues.apache.org/jira/browse/MESOS-4439
https://issues.apache.org/jira/browse/MESOS-4575


Repository: mesos


Description
---

Image cache will cache metadata about all Appc images stores in the store's
image directory. This cache could be useful to:
  - Quickly query if an image is present in the store.
  - By sharing the cache with other components like fetcher, an image can be
  checked if its present before fetching it.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/appc/cache.hpp 
4a63d479d3328605bac08fddffe190dbe21ad2b7 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
af69db3cdfea05c72ecc6ed18adc9ce520ecdd7e 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
1f9b573f9388aafff3304358b8822a48075afb44 
  src/tests/containerizer/provisioner_appc_tests.cpp 
012dba4e24b9a94dc8da0d329baf4bec2d33ffca 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43127: Introduced Appc image cache.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 97)


Indentation.



src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 101)


Do you need this temp variable?



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 51)


Hum, copying the cache sounds weired to me since we get rid of the 
shared_ptr. Can we use Owned here instead?

That means you need to return Try for Cache::create.



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 148)


This should be Option?



src/slave/containerizer/mesos/provisioner/appc/store.cpp (lines 156 - 179)


I guess this is fine for now. The logics will change soon as we're adding 
fetching support. I think what we should do here in the future is:
```
1. get imageId as you did
2. if imageId is not found, call fetch using 'appc', move it to store and 
goto 4
3. if imageId is found, check if it's in the storeDir (cache.get(imageId))
4. If yes, fetch its dependencies
5. If not, goto 2
```

The validation will be in step 2 after a layer is downloaded.



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 168)


No need for this temp var.



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


No need for this temp var.


- Jie Yu


On Feb. 11, 2016, 4:15 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43127/
> ---
> 
> (Updated Feb. 11, 2016, 4:15 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4439 and MESOS-4575
> https://issues.apache.org/jira/browse/MESOS-4439
> https://issues.apache.org/jira/browse/MESOS-4575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Image cache will cache metadata about all Appc images stores in the store's
> image directory. This cache could be useful to:
>   - Quickly query if an image is present in the store.
>   - By sharing the cache with other components like fetcher, an image can be
>   checked if its present before fetching it.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/appc/cache.hpp 
> 4a63d479d3328605bac08fddffe190dbe21ad2b7 
>   src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
> af69db3cdfea05c72ecc6ed18adc9ce520ecdd7e 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> 1f9b573f9388aafff3304358b8822a48075afb44 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 012dba4e24b9a94dc8da0d329baf4bec2d33ffca 
> 
> Diff: https://reviews.apache.org/r/43127/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43127: Introduced Appc image cache.

[Jojy Varghese]

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

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


Review request for mesos and Jie Yu.


Changes
---

review addressed.


Bugs: MESOS-4439 and MESOS-4575
https://issues.apache.org/jira/browse/MESOS-4439
https://issues.apache.org/jira/browse/MESOS-4575


Repository: mesos


Description
---

Image cache will cache metadata about all Appc images stores in the store's
image directory. This cache could be useful to:
  - Quickly query if an image is present in the store.
  - By sharing the cache with other components like fetcher, an image can be
  checked if its present before fetching it.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/appc/cache.hpp 
4a63d479d3328605bac08fddffe190dbe21ad2b7 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
af69db3cdfea05c72ecc6ed18adc9ce520ecdd7e 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
1f9b573f9388aafff3304358b8822a48075afb44 
  src/tests/containerizer/provisioner_appc_tests.cpp 
012dba4e24b9a94dc8da0d329baf4bec2d33ffca 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43127: Introduced Appc image cache.

[Jojy Varghese]

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

(Updated Feb. 10, 2016, 3:13 p.m.)


Review request for mesos and Jie Yu.


Changes
---

changes as per discussion


Bugs: MESOS-4439 and MESOS-4575
https://issues.apache.org/jira/browse/MESOS-4439
https://issues.apache.org/jira/browse/MESOS-4575


Repository: mesos


Description
---

Image cache will cache metadata about all Appc images stores in the store's
image directory. This cache could be useful to:
  - Quickly query if an image is present in the store.
  - By sharing the cache with other components like fetcher, an image can be
  checked if its present before fetching it.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/appc/cache.hpp 
4a63d479d3328605bac08fddffe190dbe21ad2b7 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
af69db3cdfea05c72ecc6ed18adc9ce520ecdd7e 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
1f9b573f9388aafff3304358b8822a48075afb44 
  src/tests/containerizer/provisioner_appc_tests.cpp 
012dba4e24b9a94dc8da0d329baf4bec2d33ffca 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43127: Introduced Appc image cache.

[Jie Yu]

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




src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 23)


we don't include protobuf header directly. Can you include 
 instead?



src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 25)


Can you put stout headers above mesos headers.



src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 58)


Please update the comments.



src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 62 - 63)


Hum, I don't like the fact that we do the validation here. I think the 
validation should be done before it was moved to storeDir/layers.



src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 83 - 86)


We try to avoid over commenting. The name of the class and the fields in 
this class is pretty descriptive. So no need for the verbose comment here.



src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 89 - 91)


Kill this as well.



src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 94 - 96)


Move the parameters up
```
Key(const std::string& name,
const hashmap labels)
```



src/slave/containerizer/mesos/provisioner/appc/cache.hpp (lines 104 - 106)


Kill this as well.



src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 116)


Add a new line above.



src/slave/containerizer/mesos/provisioner/appc/cache.hpp (line 117)


`data` is not very descriptive. How about calling it `imageIds`



src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 66)


s/addCache/adding/



src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 73)


typo. Please turn on spell check in your editor.

s/Cache//



src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 84)


As I suggested previously, please do not put validate logics in createEntry 
since the image should already been validated before it's moved to 'storeDir'. 
This is actually a pretty important invariant we rely on.

Also, no need for the helper 'createEntry' as 'add' suggests that it'll 
create the entry.



src/slave/containerizer/mesos/provisioner/appc/cache.cpp (lines 193 - 203)


This is problematic. My understanding is that boost::hash_combine is not 
commutative. You'll have to sort it before calling hash_combine. Or, a better 
alternative is to use std::map for 'labels' so that it's sorted by default.



src/slave/containerizer/mesos/provisioner/appc/cache.cpp (line 200)


can you just call two hash_combine here?
```
hash_combine(seed, name);
hash_combine(seed, value);
```



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 90)


s/createCache/cache/



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 91)


Kill this line.



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 136)


typo.



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


Let need for the helper function here. Please just inline it.



src/tests/containerizer/provisioner_appc_tests.cpp (line 222)


s/getDefaultImage/getTestImage/



src/tests/containerizer/provisioner_appc_tests.cpp (line 224)


s/imageInfo/appc.



src/tests/containerizer/provisioner_appc_tests.cpp (line 230)


s/versionLabel/version/

Ditto for below


- Jie Yu


On Feb. 10, 2016, 3:13 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43127/
> ---
> 
> (Updated 

Re: Review Request 43127: Introduced Appc image cache.

[Jojy Varghese]

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

(Updated Feb. 11, 2016, 1:14 a.m.)


Review request for mesos and Jie Yu.


Changes
---

addressed review.


Bugs: MESOS-4439 and MESOS-4575
https://issues.apache.org/jira/browse/MESOS-4439
https://issues.apache.org/jira/browse/MESOS-4575


Repository: mesos


Description
---

Image cache will cache metadata about all Appc images stores in the store's
image directory. This cache could be useful to:
  - Quickly query if an image is present in the store.
  - By sharing the cache with other components like fetcher, an image can be
  checked if its present before fetching it.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/appc/cache.hpp 
4a63d479d3328605bac08fddffe190dbe21ad2b7 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
af69db3cdfea05c72ecc6ed18adc9ce520ecdd7e 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
1f9b573f9388aafff3304358b8822a48075afb44 
  src/tests/containerizer/provisioner_appc_tests.cpp 
012dba4e24b9a94dc8da0d329baf4bec2d33ffca 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43127: Introduced Appc image cache.

[Jojy Varghese]

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

(Updated Feb. 9, 2016, 8:25 p.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


Bugs: MESOS-4439 and MESOS-4575
https://issues.apache.org/jira/browse/MESOS-4439
https://issues.apache.org/jira/browse/MESOS-4575


Repository: mesos


Description
---

Image cache will cache metadata about all Appc images stores in the store's
image directory. This cache could be useful to:
  - Quickly query if an image is present in the store.
  - By sharing the cache with other components like fetcher, an image can be
  checked if its present before fetching it.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/appc/cache.hpp 
4a63d479d3328605bac08fddffe190dbe21ad2b7 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
af69db3cdfea05c72ecc6ed18adc9ce520ecdd7e 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
1f9b573f9388aafff3304358b8822a48075afb44 
  src/tests/containerizer/provisioner_appc_tests.cpp 
012dba4e24b9a94dc8da0d329baf4bec2d33ffca 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43127: Introduced Appc image cache.

[Jojy Varghese]

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

(Updated Feb. 6, 2016, 11:11 p.m.)


Review request for mesos and Jie Yu.


Changes
---

rebase; added imageid to cache key.


Bugs: MESOS-4439 and MESOS-4575
https://issues.apache.org/jira/browse/MESOS-4439
https://issues.apache.org/jira/browse/MESOS-4575


Repository: mesos


Description
---

Image cache will cache metadata about all Appc images stores in the store's
image directory. This cache could be useful to:
  - Quickly query if an image is present in the store.
  - By sharing the cache with other components like fetcher, an image can be
  checked if its present before fetching it.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/appc/cache.hpp 
4a63d479d3328605bac08fddffe190dbe21ad2b7 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp 
af69db3cdfea05c72ecc6ed18adc9ce520ecdd7e 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
1f9b573f9388aafff3304358b8822a48075afb44 
  src/tests/containerizer/provisioner_appc_tests.cpp 
012dba4e24b9a94dc8da0d329baf4bec2d33ffca 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43127: Introduced Appc image cache.

[Jojy Varghese]

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

(Updated Feb. 4, 2016, 2:55 a.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased.


Bugs: MESOS-4439 and MESOS-4575
https://issues.apache.org/jira/browse/MESOS-4439
https://issues.apache.org/jira/browse/MESOS-4575


Repository: mesos


Description
---

Image cache will cache metadata about all Appc images stores in the store's
image directory. This cache could be useful to:
  - Quickly query if an image is present in the store.
  - By sharing the cache with other components like fetcher, an image can be
  checked if its present before fetching it.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/appc/cache.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
73c4df858a70da3d4cc4a1cb15092165f6ff8fe4 
  src/tests/containerizer/provisioner_appc_tests.cpp 
e3d08d9e49df93d5290099c8bfd917f60c93e51b 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Review Request 43127: Introduced Appc image cache.

[Jojy Varghese]

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

Review request for mesos and Jie Yu.


Bugs: MESOS-4439 and MESOS-4575
https://issues.apache.org/jira/browse/MESOS-4439
https://issues.apache.org/jira/browse/MESOS-4575


Repository: mesos


Description
---

Image cache will cache metadata about all Appc images stores in the store's
image directory. This cache could be useful to:
  - Quickly query if an image is present in the store.
  - By sharing the cache with other components like fetcher, an image can be
  checked if its present before fetching it.


Diffs
-

  src/slave/containerizer/mesos/provisioner/appc/cache.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
73c4df858a70da3d4cc4a1cb15092165f6ff8fe4 
  src/tests/containerizer/provisioner_appc_tests.cpp 
e3d08d9e49df93d5290099c8bfd917f60c93e51b 

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


Testing
---

make check.


Thanks,

Jojy Varghese