mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 37996: Added property manager
Date Fri, 25 Sep 2015 09:12:44 GMT

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


Looks like a reasonable start to a useful class. I feel like there should be some existing
code/data-structure that would handle this use case, but it's not boost's PropertyTree, so
I don't know what it would be. Several nits, but the biggest change would be switching over
to use stout Path (although you might have to build it up to get value out of it).


3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 14)
<https://reviews.apache.org/r/37996/#comment157799>

    Why is this properties.hpp instead of pathinheritedproperties.hpp? Are there other kinds
of properties you see going in here?



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 48)
<https://reviews.apache.org/r/37996/#comment157792>

    Comment should mention the thread-safety (or lack thereof) of this data structure.



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (lines 63 - 64)
<https://reviews.apache.org/r/37996/#comment157780>

    "non well formed" => "malformed"



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 67)
<https://reviews.apache.org/r/37996/#comment157779>

    Why use a string instead of a stout::Path?



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 118)
<https://reviews.apache.org/r/37996/#comment157782>

    s/not/no/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 163)
<https://reviews.apache.org/r/37996/#comment157784>

    s/is binary operation/is a binary operation/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (lines 189 - 190)
<https://reviews.apache.org/r/37996/#comment157785>

    Only need 1 blank line here



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 191)
<https://reviews.apache.org/r/37996/#comment157786>

    s/exists/exist/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 192)
<https://reviews.apache.org/r/37996/#comment157795>

    But it doesn't actually set the value, it just creates a node and lets the caller set
the value with copy/move semantics.



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 256)
<https://reviews.apache.org/r/37996/#comment157793>

    s/three/tree/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 276)
<https://reviews.apache.org/r/37996/#comment157796>

    I don't understand "make current an property holding node". What is "current"?
    Also, s/an property/a property/



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 329)
<https://reviews.apache.org/r/37996/#comment157797>

    "The root node is the only one without a parent."



3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp (line 377)
<https://reviews.apache.org/r/37996/#comment157798>

    Superfluous comment, since `init` is passed in.



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (line 31)
<https://reviews.apache.org/r/37996/#comment157787>

    Bug? s/left/right/?



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (line 45)
<https://reviews.apache.org/r/37996/#comment157788>

    s/checkes/checks/
    s/queriend/querying/



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (line 47)
<https://reviews.apache.org/r/37996/#comment157789>

    s/parent/ancestor/?



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (lines 68 - 71)
<https://reviews.apache.org/r/37996/#comment157790>

    Duplicate?



3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp (line 111)
<https://reviews.apache.org/r/37996/#comment157791>

    s/accu/accumulator/ for readability. It's not like you have to type it much.


- Adam B


On Sept. 23, 2015, 4:17 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37996/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2015, 4:17 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3231
>     https://issues.apache.org/jira/browse/MESOS-3231
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduces the `PathInheritedProperties` class which allows to create a tree where nodes
can be tag with some property. This property is then inherited by children nodes.
> 
> Two behaviors are implemented, overriding, i.e. Adding a property to the child node of
another node with a property already will result in the ancestor property being lost. The
second behavior, accumulating, takes a function and accumulates
> properties of all ancestors.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 76e1674e08bbe65a4fdf86731823a61f231d6d12

>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 9e9c3119ad18f4cbc70c70095c71dc4fd19553df

>   3rdparty/libprocess/3rdparty/stout/include/stout/properties.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt baa648aacfd5204c33e102b58126862729fbb612

>   3rdparty/libprocess/3rdparty/stout/tests/properties_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37996/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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