Re: Review Request 70161: Added validation for `QuotaConfig`.

2019-03-12 Thread Meng Zhu 


> On March 8, 2019, 9:45 a.m., Benjamin Mahler wrote:
> > src/master/quota.cpp
> > Lines 328-331 (patched)
> > 
> >
> > Maybe also check isnormal?

Checked as part of the input scalar validation.


- Meng


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


On March 12, 2019, 11:41 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70161/
> ---
> 
> (Updated March 12, 2019, 11:41 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9596
> https://issues.apache.org/jira/browse/MESOS-9596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A `QuotaConfig` is valid if the following conditions are met:
> 
>  (1) The config has a valid non-"*" role.
> 
>  (2) Resource scalar values are non-negative and finite.
> 
>  (3) If both guarantees and limits are set for a particular
>  resource, then guarantee <= limit for that resource.
> 
> 
> Diffs
> -
> 
>   src/master/quota.hpp 5cd2bb0e8669ee0290a4c1fe1058b87251772939 
>   src/master/quota.cpp 671626c01ada595d7557d5266e39a17cce243b94 
> 
> 
> Diff: https://reviews.apache.org/r/70161/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> dedicated test added in subsequent patches
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 70203: Updated a test for `UPDATE_QUOTA` call validation.

2019-03-12 Thread Meng Zhu 

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Updated a test for `UPDATE_QUOTA` call validation.


Diffs
-

  src/tests/master_validation_tests.cpp 
9568d39accb00eec68b622fdc7f02249fefbc063 


Diff: https://reviews.apache.org/r/70203/diff/1/


Testing
---

make check


Thanks,

Meng Zhu

Review Request 70202: Added call handler for `UPDATE_QUOTA`.

2019-03-12 Thread Meng Zhu 

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

The current implementation is incomplete and only does call
validation. It returns 501 `NOT_IMPLEMENTED`.

Further work includes authorization and actual enforcement
of the quota in the allocator.


Diffs
-

  src/master/master.hpp 953cc5b8ab6a8e1920a3ad63fb2dd6382e3603ec 
  src/master/quota_handler.cpp 8d417a9b2a2fa0d83d98d65167adfada2994f75c 


Diff: https://reviews.apache.org/r/70202/diff/1/


Testing
---

make check
dedicated test added in subsequent patches


Thanks,

Meng Zhu

Re: Review Request 70161: Added validation for `QuotaConfig`.

2019-03-12 Thread Meng Zhu 

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

(Updated March 12, 2019, 11:41 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed Ben's comment.


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


Repository: mesos


Description
---

A `QuotaConfig` is valid if the following conditions are met:

 (1) The config has a valid non-"*" role.

 (2) Resource scalar values are non-negative and finite.

 (3) If both guarantees and limits are set for a particular
 resource, then guarantee <= limit for that resource.


Diffs (updated)
-

  src/master/quota.hpp 5cd2bb0e8669ee0290a4c1fe1058b87251772939 
  src/master/quota.cpp 671626c01ada595d7557d5266e39a17cce243b94 


Diff: https://reviews.apache.org/r/70161/diff/2/

Changes: https://reviews.apache.org/r/70161/diff/1-2/


Testing
---

make check
dedicated test added in subsequent patches


Thanks,

Meng Zhu

Re: Review Request 70161: Added validation for `QuotaConfig`.

2019-03-12 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: [70161, 70201, 70152, 70151, 70150, 70149, 70148, 70063, 
70069, 70062, 70061, 70160, 70159]

Error:
Circular dependency detected for review 70152.Please fix the 'depends_on' field.

- Mesos Reviewbot


On March 8, 2019, 7:25 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70161/
> ---
> 
> (Updated March 8, 2019, 7:25 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9596
> https://issues.apache.org/jira/browse/MESOS-9596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A `QuotaConfig` is valid if the following conditions are met:
> 
>  (1) The config has a valid non-"*" role.
> 
>  (2) Resource scalar values are non-negative and finite.
> 
>  (3) If both guarantees and limits are set for a particular
>  resource, then guarantee <= limit for that resource.
> 
> 
> Diffs
> -
> 
>   src/master/quota.hpp 5cd2bb0e8669ee0290a4c1fe1058b87251772939 
>   src/master/quota.cpp 671626c01ada595d7557d5266e39a17cce243b94 
> 
> 
> Diff: https://reviews.apache.org/r/70161/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> dedicated test added in subsequent patches
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 70201: Added a method to check `ResourceLimits::contains(ResourceQuantities)`.

2019-03-12 Thread Meng Zhu 

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Also added tests.


Diffs
-

  src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
  src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
  src/tests/resource_quantities_tests.cpp 
435a4949b95e9a83be73781388eb4be9c7da695b 


Diff: https://reviews.apache.org/r/70201/diff/1/


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 70199: Replaced `.get().get()` with `**` in `result.hpp`.

2019-03-12 Thread Joseph Wu 

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


Ship it!




Ship It!

- Joseph Wu


On March 12, 2019, 5:55 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70199/
> ---
> 
> (Updated March 12, 2019, 5:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Besides readability, this also avoids the lint warning.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/result.hpp 
> d5dbf6c41bf657827ed53ff7ccafba99221fa51d 
> 
> 
> Diff: https://reviews.apache.org/r/70199/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70161: Added validation for `QuotaConfig`.

2019-03-12 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [70061, 70062, 70069, 70063, 70148, 70149, 70150, 70151, 
70152, 70159, 70160, 70161]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 8, 2019, 7:25 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70161/
> ---
> 
> (Updated March 8, 2019, 7:25 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9596
> https://issues.apache.org/jira/browse/MESOS-9596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A `QuotaConfig` is valid if the following conditions are met:
> 
>  (1) The config has a valid non-"*" role.
> 
>  (2) Resource scalar values are non-negative and finite.
> 
>  (3) If both guarantees and limits are set for a particular
>  resource, then guarantee <= limit for that resource.
> 
> 
> Diffs
> -
> 
>   src/master/quota.hpp 5cd2bb0e8669ee0290a4c1fe1058b87251772939 
>   src/master/quota.cpp 671626c01ada595d7557d5266e39a17cce243b94 
> 
> 
> Diff: https://reviews.apache.org/r/70161/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> dedicated test added in subsequent patches
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 70199: Replaced `.get().get()` with `**` in `result.hpp`.

2019-03-12 Thread Meng Zhu 

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

Review request for mesos, Benjamin Bannier and Benjamin Mahler.


Repository: mesos


Description
---

Besides readability, this also avoids the lint warning.


Diffs
-

  3rdparty/stout/include/stout/result.hpp 
d5dbf6c41bf657827ed53ff7ccafba99221fa51d 


Diff: https://reviews.apache.org/r/70199/diff/1/


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 70117: Added unit tests for offer operation feedback metrics.

2019-03-12 Thread Joseph Wu 

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




src/tests/storage_local_resource_provider_tests.cpp
Lines 3832-3845 (original), 3833-3846 (patched)


In cases like this, it would be more appropriate to use `Metrics()` since 
we'd otherwise make multiple calls to `/metrics/snapshot` for each metric.

Alternatively, you could add a verion of the `metricsEqual` which takes a 
map.  The downside of a helper is that reporting which metric(s) don't match 
would be harder.

... also you forgot to delete `snapshot = Metrics();` ;)


- Joseph Wu


On March 11, 2019, 12:15 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70117/
> ---
> 
> (Updated March 11, 2019, 12:15 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
> https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a set of checks to verify the metrics introduced
> in the previous commit are working as intended.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_operation_feedback_tests.cpp 
> 5a8f54c7c53272e90ed5fa6366e8154cedf1375f 
>   src/tests/api_tests.cpp f241064dc8597972299a424958e759588f7e4fd2 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 002be27bf0731e2dba8937697b347cd1dd0a 
>   src/tests/master_tests.cpp 5a926831734e6acf0388a63dac3ea3559b44a6a9 
>   src/tests/operation_reconciliation_tests.cpp 
> 6a815ad694e2a608ce324715c920833f825793a0 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 40d7e6a30c9c11eb84f4bd5aca92cfcecb3e0eb7 
>   src/tests/reservation_endpoints_tests.cpp 
> b1897592797c40574de7995b2335f2b4bc5fc699 
>   src/tests/scheduler_tests.cpp 5fb696061248c877bfa86727f146051aee26cb58 
>   src/tests/slave_tests.cpp 5ee5609af0861e9aecf02a5eaefafe137bd9b843 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 7945384867f26fa15dc734a235ae509d5d6d350f 
> 
> 
> Diff: https://reviews.apache.org/r/70117/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 70198: Added validation method for input scalar values.

2019-03-12 Thread Meng Zhu 

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

The method validates the input scalar values to be
non-negative normal values ("normal" as defined by
`std::fpclassify`).

Also refactored some related validation logic and tests.


Diffs
-

  src/common/validation.hpp d09528f56e8b3675d677159c597399688019e65e 
  src/common/validation.cpp b622a3284cfc528f8245b0300df667d525b7eea3 
  src/common/values.cpp c788302b928747a1aed66efd4d654711410928f1 
  src/tests/common_validation_tests.cpp 
214b993fa1942f15cfb4c732a5d280274d0512e9 
  src/tests/master_validation_tests.cpp 
9568d39accb00eec68b622fdc7f02249fefbc063 
  src/v1/values.cpp 5fd9dc5a03ec7b8f6ed31acb3f868764a45b2bfd 


Diff: https://reviews.apache.org/r/70198/diff/1/


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 70160: Added `<<` operator to `protobuf::Map`.

2019-03-12 Thread Meng Zhu 


> On March 8, 2019, 9:33 a.m., Benjamin Mahler wrote:
> > include/mesos/type_utils.hpp
> > Lines 533-545 (patched)
> > 
> >
> > Can we follow the existing map formats?
> > 
> > 
> > https://github.com/apache/mesos/blob/996862828ca9b7675e40b495fe24d95615bb832b/3rdparty/stout/include/stout/stringify.hpp#L140-L192

Updated.


- Meng


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


On March 7, 2019, 11:22 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70160/
> ---
> 
> (Updated March 7, 2019, 11:22 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `<<` operator to `protobuf::Map`.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 630f21f34c2215092e5ca14ffc5c966cce2b63e4 
> 
> 
> Diff: https://reviews.apache.org/r/70160/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70152: Added tests for `ResourceLimits`.

2019-03-12 Thread Meng Zhu 

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

(Updated March 12, 2019, 4:40 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed Ben's comment.
And rebased and updated the tests according to changes made the 
`ResourceLimits` class.
In particular, added tests to verify that duplicate names are not allowed in 
string parsing.


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


Repository: mesos


Description
---

Added tests for `ResourceLimits`.


Diffs (updated)
-

  src/tests/resource_quantities_tests.cpp 
435a4949b95e9a83be73781388eb4be9c7da695b 


Diff: https://reviews.apache.org/r/70152/diff/2/

Changes: https://reviews.apache.org/r/70152/diff/1-2/


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 70151: Added `class ResourceLimits`.

2019-03-12 Thread Meng Zhu 


> On March 11, 2019, 3:49 p.m., Benjamin Mahler wrote:
> > Looks good, just the same comments as the earlier review for 
> > ResourceQuantities

Thanks. The patch has been updated.


> On March 11, 2019, 3:49 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 334 (patched)
> > 
> >
> > Ideally auto&& or const auto&, but as with the other review, probably 
> > can change 'get' to return non-Option Value::Scalar or remove it until 
> > needed?

I kept the method for symmetry with `ResourceQuantities`. I think it will be 
needed very soon in the allocator. Also, I kept the `Option` interface and 
noted that `None()` implies no-limit i.e. infinite.


- Meng


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


On March 6, 2019, 11:56 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70151/
> ---
> 
> (Updated March 6, 2019, 11:56 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Similar to `ResourceQuantities`. `ResourceLimits`
> is an efficient collection of resource quantities.
> The main difference lies in the absence semantic.
> Absent entry in `ResourceQuantities` implies zero
> quantity while in `ResourceLimits`, it implies no
> limit (i.e. infinite amount). Also, due to the
> absence semantic, zero values in `ResourceLimits`
> are not dropped.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
> 
> 
> Diff: https://reviews.apache.org/r/70151/diff/1/
> 
> 
> Testing
> ---
> 
> test added in r/70152
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70149: Added a `contains` method in `ResourceQuantities`.

2019-03-12 Thread Meng Zhu 

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

(Updated March 12, 2019, 4:30 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed Ben's comment.


Repository: mesos


Description (updated)
---

The method checks if the ResourceQuantities object
contains another ResourceQuantities.

Also added tests.


Diffs (updated)
-

  src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
  src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
  src/tests/resource_quantities_tests.cpp 
435a4949b95e9a83be73781388eb4be9c7da695b 


Diff: https://reviews.apache.org/r/70149/diff/2/

Changes: https://reviews.apache.org/r/70149/diff/1-2/


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

2019-03-12 Thread Meng Zhu 


> On March 11, 2019, 3:28 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 143 (patched)
> > 
> >
> > naming wise, perhaps 'this' and 'other' / 'otherIndex' or 'left' and 
> > 'right' / 'rightIndex' for these functions?

Done.


- Meng


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


On March 12, 2019, 4:17 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 12, 2019, 4:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

2019-03-12 Thread Meng Zhu 


> On March 11, 2019, 3:35 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 153 (patched)
> > 
> >
> > Maybe some one-liner comments on these conditions to help the reader? 
> > E.g.
> > 
> > // Right contains item not in left.

Done.


- Meng


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


On March 12, 2019, 4:17 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 12, 2019, 4:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

2019-03-12 Thread Meng Zhu 

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

(Updated March 12, 2019, 4:17 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed Ben's comment.


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


Repository: mesos


Description
---

This patch removed the map interface of
`class ResourceQuantities`, added a few built-in
arithmetic operations. Now, absent resource items imply
there is no (zero) such resources.

Also added a to-do to add `class ResourceLimits` which
is similar but treats absent resource entries as having
infinite amount of such resource.

Also changed affected call sites and tests accordingly.


Diffs (updated)
-

  src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
  src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
  src/master/allocator/sorter/drf/sorter.hpp 
e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
  src/master/allocator/sorter/drf/sorter.cpp 
b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
  src/master/allocator/sorter/random/sorter.hpp 
4f230ec740e2f80d5333c61c5b23d9a631bdb273 
  src/master/allocator/sorter/random/sorter.cpp 
f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
  src/tests/resource_quantities_tests.cpp 
435a4949b95e9a83be73781388eb4be9c7da695b 


Diff: https://reviews.apache.org/r/70062/diff/4/

Changes: https://reviews.apache.org/r/70062/diff/3-4/


Testing
---

make check
Dedicated tests are added in the subsequent patch.


Thanks,

Meng Zhu

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

2019-03-12 Thread Meng Zhu 


> On March 11, 2019, 3:15 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 109-111 (original), 112-115 (patched)
> > 
> >
> > Do we still need this now that we're splitting the class? Maybe remove 
> > it for now or make it return non-option `Value::Scalar` and None becomes 0?

yep, still needed e.g. 
https://github.com/apache/mesos/blob/0dc24dba9afdca1948eea5cad05f861162dc8dd6/src/master/allocator/sorter/drf/sorter.cpp#L675
I made it non-option and `None` becomes 0.


- Meng


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


On March 12, 2019, 4:17 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 12, 2019, 4:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70186: Updated protobuf comments related to operation feedback.

2019-03-12 Thread Joseph Wu 

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


Ship it!




Ship It!

- Joseph Wu


On March 11, 2019, 12:18 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70186/
> ---
> 
> (Updated March 11, 2019, 12:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While the `id` field in `Offer::Operation` was previously marked
> experimental, it is also wise to explicitly mark the related
> scheduler API calls and events as experimental to avoid any
> confusion.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 099873278bfc4ca0f28ce52f329cba0903cec7cb 
>   include/mesos/scheduler/scheduler.proto 
> 27634153b372584a214c9631a939254bd984c735 
>   include/mesos/v1/mesos.proto 3656aa7f9d87ff3868bbc7b221d9fc7c5d7a00a3 
>   include/mesos/v1/scheduler/scheduler.proto 
> 9b0f54604f5b7b904b52b7f0a24c3a6402659ef0 
> 
> 
> Diff: https://reviews.apache.org/r/70186/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70116: Added metrics for offer operation feedback.

2019-03-12 Thread Joseph Wu 

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



One discussion point, but otherwise LGTM.


src/master/master.cpp
Lines 11361-11385 (original), 11378-11402 (patched)


I'm curious what would be the proper way to handle operation 
cleanup/removal.

When an operation is transitioned into a terminal state, the master will 
usually `removeOperation(...)` shortly afterwards.  Since we don't decrement 
the metrics in this case, the number of terminal operations will continue to 
grow.  This seems like the proper behavior.

However, in this code, it is possible to remove an agent with non-terminal 
operations.  This means the non-terminal metrics will never be decremented.  So 
you can have a cluster with 0 operations, but the metric for pending operations 
might be non-zero.


- Joseph Wu


On March 11, 2019, 12:14 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70116/
> ---
> 
> (Updated March 11, 2019, 12:14 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
> https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds additional metrics counting the
> number of operations in each state.
> 
> Unit tests are added in the subsequent commit.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp dc68fc324de7242737123015fbac19a2129778ce 
>   src/master/metrics.hpp 4495e65b6bb11f7236335a702c4f61e7c3f9b0aa 
>   src/master/metrics.cpp 4dd73fb18a06ce8f75c4c1435dba84ade123bee9 
> 
> 
> Diff: https://reviews.apache.org/r/70116/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

2019-03-12 Thread Chun-Hung Hsiao 

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

(Updated March 12, 2019, 8:43 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Specified the set of services in SLRP.


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


Repository: mesos


Description
---

Refactored SLRP to use `ServiceManager`.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
fea623c292158deb1b4b4b9ab1ac208031471519 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 


Diff: https://reviews.apache.org/r/70169/diff/2/

Changes: https://reviews.apache.org/r/70169/diff/1-2/


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70168: Added `ServiceManager` to manage CSI plugin container lifecycles.

2019-03-12 Thread Chun-Hung Hsiao 

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

(Updated March 12, 2019, 6:59 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Removed unnecessary namespace qualifications, named the actor, and let the set 
of services be specified.


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


Repository: mesos


Description
---

The `ServiceManager` is agnostic to CSI versions, so can be used to
manage plugin containers for both CSI v0 and v1 plugins. Most of its
logic are adapted from the SLRP code.

We also separate the CSI plugin metrics from SLRP metrics object so the
metrics can be accessed by `ServiceManager`. However, we do not make
`ServiceManager` own the CSI plugin metrics object because eventually we
would like to decouple metrics lifecycles from SLRP lifecycles.


Diffs (updated)
-

  src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
  src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
  src/csi/metrics.hpp PRE-CREATION 
  src/csi/metrics.cpp PRE-CREATION 
  src/csi/service_manager.hpp PRE-CREATION 
  src/csi/service_manager.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70168/diff/4/

Changes: https://reviews.apache.org/r/70168/diff/3-4/


Testing
---

Testing done later in chain.


Thanks,

Chun-Hung Hsiao

[GitHub] [mesos] kaysoky commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
kaysoky commented on a change in pull request #326: MESOS-6874: Validate the 
match between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264819458
 
 

 ##
 File path: src/tests/containerizer/composing_containerizer_tests.cpp
 ##
 @@ -57,6 +57,32 @@ namespace tests {
 
 class ComposingContainerizerTest : public MesosTest {};
 
+TEST_F(ComposingContainerizerTest, FailsOnContainerInfoTypeMismatch)
 
 Review comment:
   I'm referring to the how the containerizers are passed messages.  When the 
`--containerizers` flag only has a single entry (`mesos` or `docker`), the 
`ComposingContainerizer` is not used.  That means the validation logic added 
here will not be run, even though the validation logic is still useful in these 
cases.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Review Request 70186: Updated protobuf comments related to operation feedback.

2019-03-12 Thread Gastón Kleiman 

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


Ship it!




Ship It!

- Gastón Kleiman


On March 11, 2019, 12:18 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70186/
> ---
> 
> (Updated March 11, 2019, 12:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While the `id` field in `Offer::Operation` was previously marked
> experimental, it is also wise to explicitly mark the related
> scheduler API calls and events as experimental to avoid any
> confusion.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 099873278bfc4ca0f28ce52f329cba0903cec7cb 
>   include/mesos/scheduler/scheduler.proto 
> 27634153b372584a214c9631a939254bd984c735 
>   include/mesos/v1/mesos.proto 3656aa7f9d87ff3868bbc7b221d9fc7c5d7a00a3 
>   include/mesos/v1/scheduler/scheduler.proto 
> 9b0f54604f5b7b904b52b7f0a24c3a6402659ef0 
> 
> 
> Diff: https://reviews.apache.org/r/70186/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

[GitHub] [mesos] asekretenko commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
asekretenko commented on a change in pull request #326: MESOS-6874: Validate 
the match between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264652265
 
 

 ##
 File path: src/slave/containerizer/composing.cpp
 ##
 @@ -425,6 +425,28 @@ Future 
ComposingContainerizerProcess::_launch(
 }
 
 
+static Try validateContainerInfoType(const ContainerInfo& info)
 
 Review comment:
   Making this a templated helper makes sense, I'll do that.
   
   Using this helper for all the other similar protobufs looks like another 
task of an unknown difficulty and volume;)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] asekretenko commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
asekretenko commented on a change in pull request #326: MESOS-6874: Validate 
the match between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264650408
 
 

 ##
 File path: src/tests/containerizer/composing_containerizer_tests.cpp
 ##
 @@ -57,6 +57,32 @@ namespace tests {
 
 class ComposingContainerizerTest : public MesosTest {};
 
+TEST_F(ComposingContainerizerTest, FailsOnContainerInfoTypeMismatch)
 
 Review comment:
   Actually, this test produces a validation error without feeding _any_ real 
containerizer into the `ComposingContainerizer`. Only the mock containerizer is 
used. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] asekretenko commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
asekretenko commented on a change in pull request #326: MESOS-6874: Validate 
the match between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264639836
 
 

 ##
 File path: include/mesos/mesos.proto
 ##
 @@ -3328,6 +3328,9 @@ message TTYInfo {
  */
 message ContainerInfo {
   // All container implementation types.
+  // For each type there should be a field in the ContainerInfo itself
+  // with exactly matching name in the lowercase.
 
 Review comment:
   Unfortunately, there are two problems with converting this external 
interface to `oneof`.
   - First, it would make the code that sets both `docker` and `mesos` silently 
do strange things (instead of failing). I cannot prove that there is no such 
code outside of Mesos.
   - Second, we will still need `ContainerInfo::Type` and thus the validation. 
There are a lot of tests that only do `set_type(Type::MESOS)` but set neither 
`docker` nor `mesos`. According to the comment above the `ContainerInfo` 
definition, this is a perfectly valid way to set up a `MESOS` container without 
a filesystem isolation. I would expect such code to also exist in production 
outside of Mesos itself.
   
   It would have been a right decision to use `oneof` when this interface was 
initially introduced (and do without the `type` field), but now it is too 
late:( 


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] lava commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
lava commented on a change in pull request #326: MESOS-6874: Validate the match 
between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264624389
 
 

 ##
 File path: src/tests/health_check_tests.cpp
 ##
 @@ -1889,9 +1900,12 @@ TEST_F_TEMP_DISABLED_ON_WINDOWS(
   TaskInfo task = createTask(offers->front(), "sleep 120");
 
   // TODO(tnachen): Use local image to test if possible.
+  Image image;
+  image.set_type(Image::DOCKER);
+  image.mutable_docker()->set_name("alpine");
 
 Review comment:
   I'd suggest adding a blank line here.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] lava commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
lava commented on a change in pull request #326: MESOS-6874: Validate the match 
between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264619462
 
 

 ##
 File path: include/mesos/mesos.proto
 ##
 @@ -3328,6 +3328,9 @@ message TTYInfo {
  */
 message ContainerInfo {
   // All container implementation types.
+  // For each type there should be a field in the ContainerInfo itself
+  // with exactly matching name in the lowercase.
 
 Review comment:
   `// with an exactly matching name in lowercase.`
   
   Also, I'd drop the blank line since the comment refers to the `Type` enum 
below.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] lava commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
lava commented on a change in pull request #326: MESOS-6874: Validate the match 
between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264621825
 
 

 ##
 File path: src/slave/containerizer/composing.cpp
 ##
 @@ -435,6 +457,14 @@ Future 
ComposingContainerizerProcess::launch(
 return Containerizer::LaunchResult::ALREADY_LAUNCHED;
   }
 
+  if (containerConfig.has_container_info()) {
+Try validation =
+  validateContainerInfoType(containerConfig.container_info());
 
 Review comment:
   We should put a blank line after multi-line statements.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] lava commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
lava commented on a change in pull request #326: MESOS-6874: Validate the match 
between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264623839
 
 

 ##
 File path: src/tests/containerizer/composing_containerizer_tests.cpp
 ##
 @@ -57,6 +57,32 @@ namespace tests {
 
 class ComposingContainerizerTest : public MesosTest {};
 
+TEST_F(ComposingContainerizerTest, FailsOnContainerInfoTypeMismatch)
 
 Review comment:
   I'm not sure that I understand your comment - the validation acts on the 
protobuf message, so the value of the `--containerizers` flag should not make 
any difference?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] lava commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
lava commented on a change in pull request #326: MESOS-6874: Validate the match 
between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264624869
 
 

 ##
 File path: include/mesos/mesos.proto
 ##
 @@ -3328,6 +3328,9 @@ message TTYInfo {
  */
 message ContainerInfo {
   // All container implementation types.
+  // For each type there should be a field in the ContainerInfo itself
+  // with exactly matching name in the lowercase.
 
 Review comment:
   We looked at `oneof` while writing the patches, but we cannot use it here 
because it breaks backwards compatibility.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] lava commented on a change in pull request #326: MESOS-6874: Validate the match between Type and *Infos in the ContainerInfo.

2019-03-12 Thread GitBox 
lava commented on a change in pull request #326: MESOS-6874: Validate the match 
between Type and *Infos in the ContainerInfo.
URL: https://github.com/apache/mesos/pull/326#discussion_r264618371
 
 

 ##
 File path: src/slave/containerizer/composing.cpp
 ##
 @@ -425,6 +425,28 @@ Future 
ComposingContainerizerProcess::_launch(
 }
 
 
+static Try validateContainerInfoType(const ContainerInfo& info)
+{
+  const auto* typeDescriptor = ContainerInfo::Type_descriptor();
+  const auto* infoDescriptor = ContainerInfo::descriptor();
+  for (int index = 0; index < typeDescriptor->value_count(); index++) {
+const auto* typeValueDescriptor = typeDescriptor->value(index);
+const auto* infoValueDescriptor = infoDescriptor->FindFieldByName(
+  strings::lower(typeValueDescriptor->name()));
 
 Review comment:
   In general, we use 2-space indent but for function continuation it's 4 
spaces: 
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions
   
   (the same comment applies for all the other  indentation below)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services