You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2012/11/28 00:19:17 UTC
Review Request: Disallow executors with no resources,
use resources for the default mesos executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8239/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Description
-------
We were not validating executor resources, some executors would request 0 cpus and 0 memory.
This would result in an OOM (if using the memory subsystem) or cpuset.cpus errors (if using the cpuset subsystem, we would write nothing to the cpuset.cpus file. This results in a write error: "No space left on device.").
Diffs
-----
src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
src/examples/python/test_framework.py f436865b32c555a593e7cbb2c35b8cdae439932a
src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
src/slave/constants.hpp ddf02570caf3793106b3c48e158a5bb48c1ae80c
src/slave/constants.cpp 1735a6b55a93e6537a5a119e5345961f3d84a000
src/slave/slave.hpp bbba4404e9e2b1ff1e246f017cdad704438973ba
src/slave/slave.cpp 28fd4c336d8ac658cf92811d20066a6cfdf5a95e
src/tests/allocator_tests.cpp 44d72ad2b746b1b7032f8243817a6002100a7503
src/tests/allocator_zookeeper_tests.cpp 5f83dd71aa3cac1dd3727b65af087f651f961392
src/tests/fault_tolerance_tests.cpp 7c3d155479a4f7fda5c14f62909eea5f5a1a92df
src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
src/tests/resource_offers_tests.cpp 5981191597e673c47be331420b71be66456694c6
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8239/diff/
Testing
-------
make check
we will also be deploying this to a test cluster
Thanks,
Ben Mahler
Re: Review Request: Disallow executors with no resources,
use resources for the default mesos executor.
Posted by Ben Mahler <be...@gmail.com>.
> On Nov. 27, 2012, 11:41 p.m., Vinod Kone wrote:
> > src/tests/utils.hpp, line 205
> > <https://reviews.apache.org/r/8239/diff/1/?file=231238#file231238line205>
> >
> > ditto
> On Nov. 27, 2012, 11:41 p.m., Vinod Kone wrote:
> > src/tests/utils.hpp, line 94
> > <https://reviews.apache.org/r/8239/diff/1/?file=231238#file231238line94>
> >
> > can you just do this
> >
> > ExecutorInfo executor = DEFAULT_EXECUTOR_INFO
> >
> > and eliminate the setting up resources below?
> On Nov. 27, 2012, 11:41 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 565
> > <https://reviews.apache.org/r/8239/diff/1/?file=231232#file231232line565>
> >
> > can you add a test for this case?
Added ResourceOffersTest.ExecutorUsesNoResources which looks similar to ResourceOffersTest.TaskUsesNoResources.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8239/#review13791
-----------------------------------------------------------
On Nov. 27, 2012, 11:19 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8239/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2012, 11:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> We were not validating executor resources, some executors would request 0 cpus and 0 memory.
> This would result in an OOM (if using the memory subsystem) or cpuset.cpus errors (if using the cpuset subsystem, we would write nothing to the cpuset.cpus file. This results in a write error: "No space left on device.").
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
> src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
> src/examples/python/test_framework.py f436865b32c555a593e7cbb2c35b8cdae439932a
> src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
> src/slave/constants.hpp ddf02570caf3793106b3c48e158a5bb48c1ae80c
> src/slave/constants.cpp 1735a6b55a93e6537a5a119e5345961f3d84a000
> src/slave/slave.hpp bbba4404e9e2b1ff1e246f017cdad704438973ba
> src/slave/slave.cpp 28fd4c336d8ac658cf92811d20066a6cfdf5a95e
> src/tests/allocator_tests.cpp 44d72ad2b746b1b7032f8243817a6002100a7503
> src/tests/allocator_zookeeper_tests.cpp 5f83dd71aa3cac1dd3727b65af087f651f961392
> src/tests/fault_tolerance_tests.cpp 7c3d155479a4f7fda5c14f62909eea5f5a1a92df
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/resource_offers_tests.cpp 5981191597e673c47be331420b71be66456694c6
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8239/diff/
>
>
> Testing
> -------
>
> make check
>
> we will also be deploying this to a test cluster
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Disallow executors with no resources,
use resources for the default mesos executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8239/#review13791
-----------------------------------------------------------
src/slave/slave.hpp
<https://reviews.apache.org/r/8239/#comment29468>
As discussed offline, this TODO can go now?
src/slave/slave.cpp
<https://reviews.apache.org/r/8239/#comment29469>
pull these out as constants please
src/slave/slave.cpp
<https://reviews.apache.org/r/8239/#comment29470>
can you add a test for this case?
src/tests/allocator_zookeeper_tests.cpp
<https://reviews.apache.org/r/8239/#comment29471>
now that LaunchTasks() accounts for executor resources, you should be able to use the old resource matcher right?
src/tests/utils.hpp
<https://reviews.apache.org/r/8239/#comment29472>
can you just do this
ExecutorInfo executor = DEFAULT_EXECUTOR_INFO
and eliminate the setting up resources below?
src/tests/utils.hpp
<https://reviews.apache.org/r/8239/#comment29473>
ditto
- Vinod Kone
On Nov. 27, 2012, 11:19 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8239/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2012, 11:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
>
>
> Description
> -------
>
> We were not validating executor resources, some executors would request 0 cpus and 0 memory.
> This would result in an OOM (if using the memory subsystem) or cpuset.cpus errors (if using the cpuset subsystem, we would write nothing to the cpuset.cpus file. This results in a write error: "No space left on device.").
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
> src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
> src/examples/python/test_framework.py f436865b32c555a593e7cbb2c35b8cdae439932a
> src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
> src/slave/constants.hpp ddf02570caf3793106b3c48e158a5bb48c1ae80c
> src/slave/constants.cpp 1735a6b55a93e6537a5a119e5345961f3d84a000
> src/slave/slave.hpp bbba4404e9e2b1ff1e246f017cdad704438973ba
> src/slave/slave.cpp 28fd4c336d8ac658cf92811d20066a6cfdf5a95e
> src/tests/allocator_tests.cpp 44d72ad2b746b1b7032f8243817a6002100a7503
> src/tests/allocator_zookeeper_tests.cpp 5f83dd71aa3cac1dd3727b65af087f651f961392
> src/tests/fault_tolerance_tests.cpp 7c3d155479a4f7fda5c14f62909eea5f5a1a92df
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/resource_offers_tests.cpp 5981191597e673c47be331420b71be66456694c6
> src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
>
> Diff: https://reviews.apache.org/r/8239/diff/
>
>
> Testing
> -------
>
> make check
>
> we will also be deploying this to a test cluster
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Disallow executors with no resources,
use resources for the default mesos executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8239/#review14140
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Dec. 6, 2012, 12:51 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8239/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 12:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> We were not validating executor resources, some executors would request 0 cpus and 0 memory.
> This would result in an OOM (if using the memory subsystem) or cpuset.cpus errors (if using the cpuset subsystem, we would write nothing to the cpuset.cpus file. This results in a write error: "No space left on device.").
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
> src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
> src/examples/python/test_framework.py f436865b32c555a593e7cbb2c35b8cdae439932a
> src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
> src/slave/constants.hpp 464bf3f5752a3180031d3e88350fc92385eb7ee6
> src/slave/constants.cpp PRE-CREATION
> src/slave/slave.hpp 032e1c26468d1bd7aaa204147b13545bb1384bd9
> src/slave/slave.cpp 3d5a32cc51a5b8a5ff51c89da861ad1aeb0d7b33
> src/tests/allocator_tests.cpp 44d72ad2b746b1b7032f8243817a6002100a7503
> src/tests/allocator_zookeeper_tests.cpp 5f83dd71aa3cac1dd3727b65af087f651f961392
> src/tests/fault_tolerance_tests.cpp 7c3d155479a4f7fda5c14f62909eea5f5a1a92df
> src/tests/master_tests.cpp 4e1232281e64226dbd24f2ca8d14aaa720003900
> src/tests/resource_offers_tests.cpp 5981191597e673c47be331420b71be66456694c6
> src/tests/utils.hpp cc1a81d07c6f23e3e2590c2df485f18d114cc6a6
>
> Diff: https://reviews.apache.org/r/8239/diff/
>
>
> Testing
> -------
>
> make check
>
> we will also be deploying this to a test cluster
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Disallow executors with no resources,
use resources for the default mesos executor.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8239/
-----------------------------------------------------------
(Updated Dec. 12, 2012, 9:41 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Small format change from benh review.
Description
-------
We were not validating executor resources, some executors would request 0 cpus and 0 memory.
This would result in an OOM (if using the memory subsystem) or cpuset.cpus errors (if using the cpuset subsystem, we would write nothing to the cpuset.cpus file. This results in a write error: "No space left on device.").
Diffs (updated)
-----
src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
src/examples/python/test_framework.py f436865b32c555a593e7cbb2c35b8cdae439932a
src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
src/slave/constants.hpp 464bf3f5752a3180031d3e88350fc92385eb7ee6
src/slave/constants.cpp PRE-CREATION
src/slave/slave.hpp 032e1c26468d1bd7aaa204147b13545bb1384bd9
src/slave/slave.cpp 3d5a32cc51a5b8a5ff51c89da861ad1aeb0d7b33
src/tests/allocator_tests.cpp 44d72ad2b746b1b7032f8243817a6002100a7503
src/tests/allocator_zookeeper_tests.cpp 5f83dd71aa3cac1dd3727b65af087f651f961392
src/tests/fault_tolerance_tests.cpp 7c3d155479a4f7fda5c14f62909eea5f5a1a92df
src/tests/master_tests.cpp 4e1232281e64226dbd24f2ca8d14aaa720003900
src/tests/resource_offers_tests.cpp 5981191597e673c47be331420b71be66456694c6
src/tests/utils.hpp cc1a81d07c6f23e3e2590c2df485f18d114cc6a6
Diff: https://reviews.apache.org/r/8239/diff/
Testing
-------
make check
we will also be deploying this to a test cluster
Thanks,
Ben Mahler
Re: Review Request: Disallow executors with no resources,
use resources for the default mesos executor.
Posted by Ben Mahler <be...@gmail.com>.
> On Dec. 11, 2012, 11:01 p.m., Benjamin Hindman wrote:
> > src/tests/fault_tolerance_tests.cpp, line 413
> > <https://reviews.apache.org/r/8239/diff/2/?file=234762#file234762line413>
> >
> > We've been putting DoAll's on two different lines because they seem easier to read. Any reason you pulled this (and a few others) up?
Fixed my change here, I didn't end up finding any other places where I pulled the formatting up on DoAll.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8239/#review14323
-----------------------------------------------------------
On Dec. 6, 2012, 12:51 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8239/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 12:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> We were not validating executor resources, some executors would request 0 cpus and 0 memory.
> This would result in an OOM (if using the memory subsystem) or cpuset.cpus errors (if using the cpuset subsystem, we would write nothing to the cpuset.cpus file. This results in a write error: "No space left on device.").
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
> src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
> src/examples/python/test_framework.py f436865b32c555a593e7cbb2c35b8cdae439932a
> src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
> src/slave/constants.hpp 464bf3f5752a3180031d3e88350fc92385eb7ee6
> src/slave/constants.cpp PRE-CREATION
> src/slave/slave.hpp 032e1c26468d1bd7aaa204147b13545bb1384bd9
> src/slave/slave.cpp 3d5a32cc51a5b8a5ff51c89da861ad1aeb0d7b33
> src/tests/allocator_tests.cpp 44d72ad2b746b1b7032f8243817a6002100a7503
> src/tests/allocator_zookeeper_tests.cpp 5f83dd71aa3cac1dd3727b65af087f651f961392
> src/tests/fault_tolerance_tests.cpp 7c3d155479a4f7fda5c14f62909eea5f5a1a92df
> src/tests/master_tests.cpp 4e1232281e64226dbd24f2ca8d14aaa720003900
> src/tests/resource_offers_tests.cpp 5981191597e673c47be331420b71be66456694c6
> src/tests/utils.hpp cc1a81d07c6f23e3e2590c2df485f18d114cc6a6
>
> Diff: https://reviews.apache.org/r/8239/diff/
>
>
> Testing
> -------
>
> make check
>
> we will also be deploying this to a test cluster
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Disallow executors with no resources,
use resources for the default mesos executor.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8239/#review14323
-----------------------------------------------------------
src/tests/fault_tolerance_tests.cpp
<https://reviews.apache.org/r/8239/#comment30497>
We've been putting DoAll's on two different lines because they seem easier to read. Any reason you pulled this (and a few others) up?
- Benjamin Hindman
On Dec. 6, 2012, 12:51 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8239/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 12:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> We were not validating executor resources, some executors would request 0 cpus and 0 memory.
> This would result in an OOM (if using the memory subsystem) or cpuset.cpus errors (if using the cpuset subsystem, we would write nothing to the cpuset.cpus file. This results in a write error: "No space left on device.").
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
> src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
> src/examples/python/test_framework.py f436865b32c555a593e7cbb2c35b8cdae439932a
> src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
> src/slave/constants.hpp 464bf3f5752a3180031d3e88350fc92385eb7ee6
> src/slave/constants.cpp PRE-CREATION
> src/slave/slave.hpp 032e1c26468d1bd7aaa204147b13545bb1384bd9
> src/slave/slave.cpp 3d5a32cc51a5b8a5ff51c89da861ad1aeb0d7b33
> src/tests/allocator_tests.cpp 44d72ad2b746b1b7032f8243817a6002100a7503
> src/tests/allocator_zookeeper_tests.cpp 5f83dd71aa3cac1dd3727b65af087f651f961392
> src/tests/fault_tolerance_tests.cpp 7c3d155479a4f7fda5c14f62909eea5f5a1a92df
> src/tests/master_tests.cpp 4e1232281e64226dbd24f2ca8d14aaa720003900
> src/tests/resource_offers_tests.cpp 5981191597e673c47be331420b71be66456694c6
> src/tests/utils.hpp cc1a81d07c6f23e3e2590c2df485f18d114cc6a6
>
> Diff: https://reviews.apache.org/r/8239/diff/
>
>
> Testing
> -------
>
> make check
>
> we will also be deploying this to a test cluster
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Disallow executors with no resources,
use resources for the default mesos executor.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8239/
-----------------------------------------------------------
(Updated Dec. 6, 2012, 12:51 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Description
-------
We were not validating executor resources, some executors would request 0 cpus and 0 memory.
This would result in an OOM (if using the memory subsystem) or cpuset.cpus errors (if using the cpuset subsystem, we would write nothing to the cpuset.cpus file. This results in a write error: "No space left on device.").
Diffs (updated)
-----
src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
src/examples/python/test_framework.py f436865b32c555a593e7cbb2c35b8cdae439932a
src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
src/slave/constants.hpp 464bf3f5752a3180031d3e88350fc92385eb7ee6
src/slave/constants.cpp PRE-CREATION
src/slave/slave.hpp 032e1c26468d1bd7aaa204147b13545bb1384bd9
src/slave/slave.cpp 3d5a32cc51a5b8a5ff51c89da861ad1aeb0d7b33
src/tests/allocator_tests.cpp 44d72ad2b746b1b7032f8243817a6002100a7503
src/tests/allocator_zookeeper_tests.cpp 5f83dd71aa3cac1dd3727b65af087f651f961392
src/tests/fault_tolerance_tests.cpp 7c3d155479a4f7fda5c14f62909eea5f5a1a92df
src/tests/master_tests.cpp 4e1232281e64226dbd24f2ca8d14aaa720003900
src/tests/resource_offers_tests.cpp 5981191597e673c47be331420b71be66456694c6
src/tests/utils.hpp cc1a81d07c6f23e3e2590c2df485f18d114cc6a6
Diff: https://reviews.apache.org/r/8239/diff/
Testing
-------
make check
we will also be deploying this to a test cluster
Thanks,
Ben Mahler
Re: Review Request: Disallow executors with no resources,
use resources for the default mesos executor.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8239/
-----------------------------------------------------------
(Updated Nov. 27, 2012, 11:19 p.m.)
Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu.
Description
-------
We were not validating executor resources, some executors would request 0 cpus and 0 memory.
This would result in an OOM (if using the memory subsystem) or cpuset.cpus errors (if using the cpuset subsystem, we would write nothing to the cpuset.cpus file. This results in a write error: "No space left on device.").
Diffs
-----
src/examples/balloon_framework.cpp 35bc0fe712edbf6130e10e93286c5278625be096
src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
src/examples/python/test_framework.py f436865b32c555a593e7cbb2c35b8cdae439932a
src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
src/slave/constants.hpp ddf02570caf3793106b3c48e158a5bb48c1ae80c
src/slave/constants.cpp 1735a6b55a93e6537a5a119e5345961f3d84a000
src/slave/slave.hpp bbba4404e9e2b1ff1e246f017cdad704438973ba
src/slave/slave.cpp 28fd4c336d8ac658cf92811d20066a6cfdf5a95e
src/tests/allocator_tests.cpp 44d72ad2b746b1b7032f8243817a6002100a7503
src/tests/allocator_zookeeper_tests.cpp 5f83dd71aa3cac1dd3727b65af087f651f961392
src/tests/fault_tolerance_tests.cpp 7c3d155479a4f7fda5c14f62909eea5f5a1a92df
src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
src/tests/resource_offers_tests.cpp 5981191597e673c47be331420b71be66456694c6
src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45
Diff: https://reviews.apache.org/r/8239/diff/
Testing
-------
make check
we will also be deploying this to a test cluster
Thanks,
Ben Mahler