mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 51517: Added MOUNT or PATH disk type in logging resources.
Date Thu, 08 Sep 2016 22:41:01 GMT

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




src/common/resources.cpp (line 1694)
<https://reviews.apache.org/r/51517/#comment215728>

    Use a `switch()` as is done in `ostream& operator<<(ostream& stream, const
Volume& volume)`?



src/common/resources.cpp (lines 1695 - 1697)
<https://reviews.apache.org/r/51517/#comment215746>

    I don't think we need to do the validation here. It should be already done somewhere else
and similar output operators in this file don't do this. Here because it's protobuf, if the
object didn't go through proper validation, the worst case it's going to print the default
(empty) values.



src/common/resources.cpp (lines 1701 - 1703)
<https://reviews.apache.org/r/51517/#comment215747>

    Ditto.



src/common/resources.cpp (lines 1742 - 1744)
<https://reviews.apache.org/r/51517/#comment215736>

    



src/tests/resources_tests.cpp (line 834)
<https://reviews.apache.org/r/51517/#comment215764>

    Not yours, but starting from here this is already incorrect: persistent volumes don't
have host paths. This is currently not captured by validation IIUC, just ignored by the agent.
    
    Anyways, it'll involve some refactoring and reordering to fix this test so let's do it
later.



src/tests/resources_tests.cpp (line 842)
<https://reviews.apache.org/r/51517/#comment215755>

    This output becomes pretty unreadable IMO with this many `:` and many can be optional.
    
    To make it a little bit better for this patch, how about we use a `, ` to separate source
and the rest?
    
    `disk(alice)[MOUNT:/mnt1, hadoop:/hdfs:/data:rw]:1`



src/tests/resources_tests.cpp (line 844)
<https://reviews.apache.org/r/51517/#comment215759>

    s/TYPE/type/



src/tests/resources_tests.cpp (line 846)
<https://reviews.apache.org/r/51517/#comment215760>

    Move this line to be the last line of the prevous stanza as its cleanup?



src/tests/resources_tests.cpp (line 854)
<https://reviews.apache.org/r/51517/#comment215769>

    Don't need the comment if we move `disk.mutable_disk()->clear_source();` up.



src/tests/resources_tests.cpp (line 856)
<https://reviews.apache.org/r/51517/#comment215761>

    Move this line to be the last line of the prevous stanza as its cleanup?


- Jiang Yan Xu


On Aug. 30, 2016, 3:22 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2016, 3:22 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
>     https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -----
> 
>   src/common/resources.cpp a5f5902d8f7f2757e3aee35619bff5cc3a52f29b 
>   src/tests/resources_tests.cpp 4932d95f4e78cae764b89472373e13527b4354a2 
>   src/v1/resources.cpp 172217505d80d66cb7e10b3635dc273229313601 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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