Re: Review Request 41658: Track the allocation candidates to bound the allocation queue.

[Guangya Liu]

> On 六月 27, 2016, 12:34 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 534
> > 
> >
> > I think we do not need to insert slave here but only insert slave in 
> > addSlave and remove slave in removeSlave?
> 
> James Peach wrote:
> Since ``updateSlave`` triggers an allocation for that ``slaveId``, I 
> think we have to insert the slave here too.

Yes, but I still have some question for this: If there is no new agent added, 
then what is the use of `allocationCandidates.insert(slaveId);`, seems the 
`allocationCandidates` should already include all agents in the cluster?


> On 六月 27, 2016, 12:34 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1102
> > 
> >
> > Can you please show more detail for what does `--queuedAllocations == 
> > 0` means here? Does it means that you always allocate resources if the 
> > `queuedAllocations` is 1?
> 
> James Peach wrote:
> When ``queuedAllocations`` goes to 0, that means that there are no more 
> allocation tasks in the allocator queue so we should actually perform 
> allocation (ie. there's no later task that we can leave the work for).

I saw that the `queuedAllocations` was increased in many functions, such as 
`addFramework`, `addSlave` etc to pend the allocation, but I only saw here we 
are checking when the `queuedAllocations` goes to 0. Take an example, if I have 
5 `queuedAllocations`, when can the `--queuedAllocations == 0` beome true? 
Thanks.


- Guangya


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


On 六月 22, 2016, 4:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41658/
> ---
> 
> (Updated 六月 22, 2016, 4:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When there is churn in the cluster, frequent resource allocation
> is required.  Maintain a set of allocation candidates so that we
> don't end up running the same allocation multiple times.
> 
> This review is just for feedback. Not proposing it to be merged at this time.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 86ea5a402ed67f8f22f11d5730147cd907d66a08 
>   src/master/allocator/mesos/hierarchical.cpp 
> 775182515dcb52bd873ecdf98c827320251a59c8 
> 
> Diff: https://reviews.apache.org/r/41658/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 41658: Track the allocation candidates to bound the allocation queue.

[James Peach]

> On June 27, 2016, 12:34 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 534
> > 
> >
> > I think we do not need to insert slave here but only insert slave in 
> > addSlave and remove slave in removeSlave?

Since ``updateSlave`` triggers an allocation for that ``slaveId``, I think we 
have to insert the slave here too.


> On June 27, 2016, 12:34 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1102
> > 
> >
> > Can you please show more detail for what does `--queuedAllocations == 
> > 0` means here? Does it means that you always allocate resources if the 
> > `queuedAllocations` is 1?

When ``queuedAllocations`` goes to 0, that means that there are no more 
allocation tasks in the allocator queue so we should actually perform 
allocation (ie. there's no later task that we can leave the work for).


- James


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


On June 22, 2016, 4:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41658/
> ---
> 
> (Updated June 22, 2016, 4:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When there is churn in the cluster, frequent resource allocation
> is required.  Maintain a set of allocation candidates so that we
> don't end up running the same allocation multiple times.
> 
> This review is just for feedback. Not proposing it to be merged at this time.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 86ea5a402ed67f8f22f11d5730147cd907d66a08 
>   src/master/allocator/mesos/hierarchical.cpp 
> 775182515dcb52bd873ecdf98c827320251a59c8 
> 
> Diff: https://reviews.apache.org/r/41658/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 41658: Track the allocation candidates to bound the allocation queue.

[Guangya Liu]

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




src/master/allocator/mesos/hierarchical.cpp (line 534)


I think we do not need to insert slave here but only insert slave in 
addSlave and remove slave in removeSlave?



src/master/allocator/mesos/hierarchical.cpp (line 1102)


Can you please show more detail for what does `--queuedAllocations == 0` 
means here? Does it means that you always allocate resources if the 
`queuedAllocations` is 1?


- Guangya Liu


On 六月 22, 2016, 4:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41658/
> ---
> 
> (Updated 六月 22, 2016, 4:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When there is churn in the cluster, frequent resource allocation
> is required.  Maintain a set of allocation candidates so that we
> don't end up running the same allocation multiple times.
> 
> This review is just for feedback. Not proposing it to be merged at this time.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 86ea5a402ed67f8f22f11d5730147cd907d66a08 
>   src/master/allocator/mesos/hierarchical.cpp 
> 775182515dcb52bd873ecdf98c827320251a59c8 
> 
> Diff: https://reviews.apache.org/r/41658/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 41658: Track the allocation candidates to bound the allocation queue.

[James Peach]

> On June 21, 2016, 3:25 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 368
> > 
> >
> > Instead of using a counter I think the following is equivalent but 
> > simpler:
> > 
> > ```
> > bool allocationPending;
> > ```
> > 
> > So instead of always dispatching into the queue which results in many 
> > noops, we don't dispatch if we know there is already an allocation pending 
> > in the queue. We merely update `allocationCandidates`.
> > 
> > Later after the allocation is run we reset the bool.
> > 
> > Make sense?
> 
> James Peach wrote:
> That's not the same thing. The allocation counter lets you take the the 
> *last* allocation in the queue, rather than any. For example, imagine the 
> dispatch queue contains ATTT (T is some event, A is an allocation). The 
> allocation counter turns this into ATTTA but only the last A would be 
> executed. So if T adds allocation candidates you would end up doing fewer 
> allocations.
> 
> Note that I'm not claiming that this makes a difference in practice; it 
> could well be that the boolean flag is good enough for the common cases you 
> are trying to address. Just giving the background for the counter.
> 
> Jiang Yan Xu wrote:
> Got it. The exact sequence my vary but I think the end result is the same 
> "prevent unnecessary allocation calls".
> 
> 
> Just to clarify your example:
> 
> The A in ATTT you mentioned is the batch() call that gets inserted 
> into the queue right? If I rename it as B to differentiate it from the 
> allocate() call, then:
> 
> - With the counter based algorithm: BTTT -> BTTT**A**AA**A** 
> rigth? (The bold ones are the real allocations, the first due to `force == 
> true`)
> - With the boolean based algorithm: BTTT -> BTTT**A**. To prevent 
> starvation of the batch allocation we can do an allocation synchronously in 
> batch(), so this becomes BTTT -> **A**TTT**A**.
> 
> Does it look right?

``force == true`` is for supporting the batch, so **A** and B are the same 
thing.

Say your starting queue is TTTATT. Then some cluster event triggers another 
allocation. With the boolean flag approach you would know there is an 
allocation pending so you wouldn't dispatch a new one; you would end up with 
TTT**A**TT. With the counter approach, you would dispatch, so you would do 
TTTATT**A**, which could be a win if we assume that this let us accumulate more 
allocation candidates. Keep in mind that cluster changes are getting dispatched 
onto the allocator queue, so I don't think you can have a sequence of  - 
you always have an intermediate T that would have to dispatch an A, so the 
worst case generated by the counter approach is TATATAT**A**.


> On June 21, 2016, 3:25 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1080
> > 
> >
> > This uses `true` but I don't see why batch needs to be different.
> 
> James Peach wrote:
> The force is to ensure that allocations are not starved out. If there is 
> a lot of churn and we keep delaying the actial allocation, you can imagine a 
> scenario where the allocation is deferred indefinitely. This ensures that 
> allocations occur at least at the batch interval.
> 
> Jiang Yan Xu wrote:
> I see. With the boolean-based approach it will not be deferred 
> indefinitely but to ensure we do an allocation at least per each batch 
> interval we can do allocate() synchronously in batch (in my example above). 
> That'll be a stronger guarantee than dispatching again but with `force == 
> true` right?

Yes you are right. By dispatching ``batch``, the queue could grow unbounded if 
the allocation time is longer than the batch interval. Making this synchronous 
would be better.


- James


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


On June 22, 2016, 4:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41658/
> ---
> 
> (Updated June 22, 2016, 4:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When there is churn in the cluster, frequent resource allocation
> is required.  Maintain a set of allocation candidates so that we
> don't end up running the same allocation multiple times.
> 
> This review is just for 

Re: Review Request 41658: Track the allocation candidates to bound the allocation queue.

[James Peach]

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

(Updated June 22, 2016, 4:32 a.m.)


Review request for mesos, Benjamin Mahler and Klaus Ma.


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


Repository: mesos


Description (updated)
---

When there is churn in the cluster, frequent resource allocation
is required.  Maintain a set of allocation candidates so that we
don't end up running the same allocation multiple times.

This review is just for feedback. Not proposing it to be merged at this time.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
86ea5a402ed67f8f22f11d5730147cd907d66a08 
  src/master/allocator/mesos/hierarchical.cpp 
775182515dcb52bd873ecdf98c827320251a59c8 

Diff: https://reviews.apache.org/r/41658/diff/


Testing
---

make check.


Thanks,

James Peach

Re: Review Request 41658: Track the allocation candidates to bound the allocation queue.

[Jiang Yan Xu]

> On June 21, 2016, 8:25 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 368
> > 
> >
> > Instead of using a counter I think the following is equivalent but 
> > simpler:
> > 
> > ```
> > bool allocationPending;
> > ```
> > 
> > So instead of always dispatching into the queue which results in many 
> > noops, we don't dispatch if we know there is already an allocation pending 
> > in the queue. We merely update `allocationCandidates`.
> > 
> > Later after the allocation is run we reset the bool.
> > 
> > Make sense?
> 
> James Peach wrote:
> That's not the same thing. The allocation counter lets you take the the 
> *last* allocation in the queue, rather than any. For example, imagine the 
> dispatch queue contains ATTT (T is some event, A is an allocation). The 
> allocation counter turns this into ATTTA but only the last A would be 
> executed. So if T adds allocation candidates you would end up doing fewer 
> allocations.
> 
> Note that I'm not claiming that this makes a difference in practice; it 
> could well be that the boolean flag is good enough for the common cases you 
> are trying to address. Just giving the background for the counter.

Got it. The exact sequence my vary but I think the end result is the same 
"prevent unnecessary allocation calls".


Just to clarify your example:

The A in ATTT you mentioned is the batch() call that gets inserted into the 
queue right? If I rename it as B to differentiate it from the allocate() call, 
then:

- With the counter based algorithm: BTTT -> BTTT**A**AA**A** rigth? 
(The bold ones are the real allocations, the first due to `force == true`)
- With the boolean based algorithm: BTTT -> BTTT**A**. To prevent 
starvation of the batch allocation we can do an allocation synchronously in 
batch(), so this becomes BTTT -> **A**TTT**A**.

Does it look right?


> On June 21, 2016, 8:25 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1080
> > 
> >
> > This uses `true` but I don't see why batch needs to be different.
> 
> James Peach wrote:
> The force is to ensure that allocations are not starved out. If there is 
> a lot of churn and we keep delaying the actial allocation, you can imagine a 
> scenario where the allocation is deferred indefinitely. This ensures that 
> allocations occur at least at the batch interval.

I see. With the boolean-based approach it will not be deferred indefinitely but 
to ensure we do an allocation at least per each batch interval we can do 
allocate() synchronously in batch (in my example above). That'll be a stronger 
guarantee than dispatching again but with `force == true` right?


> On June 21, 2016, 8:25 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1137
> > 
> >
> > No need to rename the method?
> 
> James Peach wrote:
> It made it clearer since there are a number of overloads of 
> ``allocate()``.

I see. But I think the current situation is not too bad and if we rename it we 
probably need to do others so it's consistent.

We currently have:

```
  // Allocate any allocatable resources.
  void allocate();

  // Allocate resources just from the specified slave.
  void allocate(const SlaveID& slaveId);

  // Allocate resources from the specified slaves.
  void allocate(const hashset& slaveIds);
```

If we rename just one we are stilling leave the other two overloaded... but I 
guess I can't see the confusion, they all do the same thing, just with 
different input, sounds like what overloads are for.


- Jiang Yan


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


On Dec. 22, 2015, 11:56 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41658/
> ---
> 
> (Updated Dec. 22, 2015, 11:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When there is churn in the cluster, frequent resource allocation
> is required.  Maintain a set of allocation candidates so that we
> don't end up running the same allocation multiple times.
> 
> This review is just for feedback. Not proposing it to be berged at this time.
> 
> 
> Diffs
> -
> 
>   

Re: Review Request 41658: Track the allocation candidates to bound the allocation queue.

[James Peach]

> On June 21, 2016, 3:25 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 368
> > 
> >
> > Instead of using a counter I think the following is equivalent but 
> > simpler:
> > 
> > ```
> > bool allocationPending;
> > ```
> > 
> > So instead of always dispatching into the queue which results in many 
> > noops, we don't dispatch if we know there is already an allocation pending 
> > in the queue. We merely update `allocationCandidates`.
> > 
> > Later after the allocation is run we reset the bool.
> > 
> > Make sense?

That's not the same thing. The allocation counter lets you take the the *last* 
allocation in the queue, rather than any. For example, imagine the dispatch 
queue contains ATTT (T is some event, A is an allocation). The allocation 
counter turns this into ATTTA but only the last A would be executed. So if 
T adds allocation candidates you would end up doing fewer allocations.

Note that I'm not claiming that this makes a difference in practice; it could 
well be that the boolean flag is good enough for the common cases you are 
trying to address. Just giving the background for the counter.


> On June 21, 2016, 3:25 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1080
> > 
> >
> > This uses `true` but I don't see why batch needs to be different.

The force is to ensure that allocations are not starved out. If there is a lot 
of churn and we keep delaying the actial allocation, you can imagine a scenario 
where the allocation is deferred indefinitely. This ensures that allocations 
occur at least at the batch interval.


> On June 21, 2016, 3:25 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1137
> > 
> >
> > No need to rename the method?

It made it clearer since there are a number of overloads of ``allocate()``.


- James


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


On Dec. 22, 2015, 7:56 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41658/
> ---
> 
> (Updated Dec. 22, 2015, 7:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When there is churn in the cluster, frequent resource allocation
> is required.  Maintain a set of allocation candidates so that we
> don't end up running the same allocation multiple times.
> 
> This review is just for feedback. Not proposing it to be berged at this time.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 86ea5a402ed67f8f22f11d5730147cd907d66a08 
>   src/master/allocator/mesos/hierarchical.cpp 
> 775182515dcb52bd873ecdf98c827320251a59c8 
> 
> Diff: https://reviews.apache.org/r/41658/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 41658: Track the allocation candidates to bound the allocation queue.

[Jiang Yan Xu]

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



We recently ran some benchmark and found this is helping with performance so 
we'd like to revive and publish the benchmark.


src/master/allocator/mesos/hierarchical.hpp (line 365)


Instead of using a counter I think the following is equivalent but simpler:

```
bool allocationPending;
```

So instead of always dispatching into the queue which results in many 
noops, we don't dispatch if we know there is already an allocation pending in 
the queue. We merely update `allocationCandidates`.

Later after the allocation is run we reset the bool.

Make sense?



src/master/allocator/mesos/hierarchical.cpp (lines 262 - 264)


Here and elsehere

```
allocationCandidates = slaves.keys(); 

// or

allocationCandidates.insert(slaveId);

// then

if (!allocationPending) {
  allocationPending = true;
  dispatch(self(), ::allocate);
}
```



src/master/allocator/mesos/hierarchical.cpp (line 1080)


This uses `true` but I don't see why batch needs to be different.



src/master/allocator/mesos/hierarchical.cpp (lines 1106 - 1107)


After the allocation is done, 

```
allocationPending = false;
allocationCandidates.clear();
```



src/master/allocator/mesos/hierarchical.cpp (line 1116)


No need to rename the method?


- Jiang Yan Xu


On Dec. 22, 2015, 11:56 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41658/
> ---
> 
> (Updated Dec. 22, 2015, 11:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When there is churn in the cluster, frequent resource allocation
> is required.  Maintain a set of allocation candidates so that we
> don't end up running the same allocation multiple times.
> 
> This review is just for feedback. Not proposing it to be berged at this time.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 86ea5a402ed67f8f22f11d5730147cd907d66a08 
>   src/master/allocator/mesos/hierarchical.cpp 
> 775182515dcb52bd873ecdf98c827320251a59c8 
> 
> Diff: https://reviews.apache.org/r/41658/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 41658: Track the allocation candidates to bound the allocation queue.

[James Peach]

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

Review request for mesos, Ben Mahler and Klaus Ma.


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


Repository: mesos


Description
---

When there is churn in the cluster, frequent resource allocation
is required.  Maintain a set of allocation candidates so that we
don't end up running the same allocation multiple times.

This review is just for feedback. Not proposing it to be berged at this time.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
86ea5a402ed67f8f22f11d5730147cd907d66a08 
  src/master/allocator/mesos/hierarchical.cpp 
775182515dcb52bd873ecdf98c827320251a59c8 

Diff: https://reviews.apache.org/r/41658/diff/


Testing
---

make check.


Thanks,

James Peach

Re: Review Request 41658: Track the allocation candidates to bound the allocation queue.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41658]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 22, 2015, 7:56 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41658/
> ---
> 
> (Updated Dec. 22, 2015, 7:56 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When there is churn in the cluster, frequent resource allocation
> is required.  Maintain a set of allocation candidates so that we
> don't end up running the same allocation multiple times.
> 
> This review is just for feedback. Not proposing it to be berged at this time.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 86ea5a402ed67f8f22f11d5730147cd907d66a08 
>   src/master/allocator/mesos/hierarchical.cpp 
> 775182515dcb52bd873ecdf98c827320251a59c8 
> 
> Diff: https://reviews.apache.org/r/41658/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>