You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2018/05/11 03:32:11 UTC

Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

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

Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
-------

Made `UriDiskProfileAdaptor` be able to update profile selectors.


Diffs
-----

  src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 


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


Testing
-------

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

Posted by Chun-Hung Hsiao <ch...@apache.org>.

> On May 11, 2018, 12:05 p.m., James DeFelice wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.cpp
> > Line 281 (original)
> > <https://reviews.apache.org/r/67078/diff/1/?file=2019745#file2019745line281>
> >
> >     this seems like a pretty aggressive change. instead of removing the attempted optimization completely, why not fix it instead?
> >     
> >     IMO it seems like it would be cheaper to do an equivalence check here and skip the subsequent notifications if nothing has changed. we don't expect profiles to change very frequently.

The equivalence operation for proto messages does not come for free in C++, and we need to either use the `MessageDifferencer` or define it by ourselves. Since this needs to be backported I'd like to make the patch as simple as possible. Dropping this issue.


- Chun-Hung


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


On May 11, 2018, 3:32 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67078/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 3:32 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8906
>     https://issues.apache.org/jira/browse/MESOS-8906
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made `UriDiskProfileAdaptor` be able to update profile selectors.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 
> 
> 
> Diff: https://reviews.apache.org/r/67078/diff/1/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

Posted by James DeFelice <ja...@gmail.com>.

> On May 11, 2018, 12:05 p.m., James DeFelice wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.cpp
> > Line 281 (original)
> > <https://reviews.apache.org/r/67078/diff/1/?file=2019745#file2019745line281>
> >
> >     this seems like a pretty aggressive change. instead of removing the attempted optimization completely, why not fix it instead?
> >     
> >     IMO it seems like it would be cheaper to do an equivalence check here and skip the subsequent notifications if nothing has changed. we don't expect profiles to change very frequently.
> 
> Chun-Hung Hsiao wrote:
>     The equivalence operation for proto messages does not come for free in C++, and we need to either use the `MessageDifferencer` or define it by ourselves. Since this needs to be backported I'd like to make the patch as simple as possible. Dropping this issue.

OK. Please add a TODO as suggested by Jie.


- James


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


On May 11, 2018, 3:32 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67078/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 3:32 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8906
>     https://issues.apache.org/jira/browse/MESOS-8906
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made `UriDiskProfileAdaptor` be able to update profile selectors.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 
> 
> 
> Diff: https://reviews.apache.org/r/67078/diff/1/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67078/#review202928
-----------------------------------------------------------




src/resource_provider/storage/uri_disk_profile_adaptor.cpp
Line 281 (original)
<https://reviews.apache.org/r/67078/#comment284986>

    this seems like a pretty aggressive change. instead of removing the attempted optimization completely, why not fix it instead?
    
    IMO it seems like it would be cheaper to do an equivalence check here and skip the subsequent notifications if nothing has changed. we don't expect profiles to change very frequently.


- James DeFelice


On May 11, 2018, 3:32 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67078/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 3:32 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8906
>     https://issues.apache.org/jira/browse/MESOS-8906
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made `UriDiskProfileAdaptor` be able to update profile selectors.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 
> 
> 
> Diff: https://reviews.apache.org/r/67078/diff/1/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67078/#review202973
-----------------------------------------------------------



PASS: Mesos patch 67078 was successfully built and tested.

Reviews applied: `['67078']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67078

- Mesos Reviewbot Windows


On May 11, 2018, 9:28 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67078/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 9:28 p.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8906
>     https://issues.apache.org/jira/browse/MESOS-8906
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made `UriDiskProfileAdaptor` be able to update profile selectors.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 
> 
> 
> Diff: https://reviews.apache.org/r/67078/diff/2/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67078/#review202979
-----------------------------------------------------------



Patch looks great!

Reviews applied: [67078]

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

- Mesos Reviewbot


On May 11, 2018, 9:28 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67078/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 9:28 p.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8906
>     https://issues.apache.org/jira/browse/MESOS-8906
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made `UriDiskProfileAdaptor` be able to update profile selectors.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 
> 
> 
> Diff: https://reviews.apache.org/r/67078/diff/2/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67078/
-----------------------------------------------------------

(Updated May 11, 2018, 9:28 p.m.)


Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


Changes
-------

Addressed James' comment by adding a TODO.


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


Repository: mesos


Description
-------

Made `UriDiskProfileAdaptor` be able to update profile selectors.


Diffs (updated)
-----

  src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 


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

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


Testing
-------

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67078/#review202900
-----------------------------------------------------------



Patch looks great!

Reviews applied: [67078]

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

- Mesos Reviewbot


On May 11, 2018, 3:32 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67078/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 3:32 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8906
>     https://issues.apache.org/jira/browse/MESOS-8906
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made `UriDiskProfileAdaptor` be able to update profile selectors.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 
> 
> 
> Diff: https://reviews.apache.org/r/67078/diff/1/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67078/#review202959
-----------------------------------------------------------


Ship it!




I am fine with this. Let's add a TODO about a potential equality check here.

- Jie Yu


On May 11, 2018, 3:32 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67078/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 3:32 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8906
>     https://issues.apache.org/jira/browse/MESOS-8906
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made `UriDiskProfileAdaptor` be able to update profile selectors.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 
> 
> 
> Diff: https://reviews.apache.org/r/67078/diff/1/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67078: Made `UriDiskProfileAdaptor` be able to update profile selectors.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67078/#review202890
-----------------------------------------------------------



PASS: Mesos patch 67078 was successfully built and tested.

Reviews applied: `['67078']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67078

- Mesos Reviewbot Windows


On May 11, 2018, 3:32 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67078/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 3:32 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8906
>     https://issues.apache.org/jira/browse/MESOS-8906
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Made `UriDiskProfileAdaptor` be able to update profile selectors.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 300ea12687de487737ce91066ab4e74d9b3430e6 
> 
> 
> Diff: https://reviews.apache.org/r/67078/diff/1/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>