You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by haosdent huang <ha...@apache.org> on 2017/04/29 18:22:04 UTC
Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
Bugs: MESOS-4992
https://issues.apache.org/jira/browse/MESOS-4992
Repository: mesos
Description
-------
Ensured sandbox URI request reroute after fetched `$scope.state`.
Diffs
-----
src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
Diff: https://reviews.apache.org/r/58872/diff/1/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by haosdent huang <ha...@apache.org>.
> On May 1, 2017, 10:45 p.m., Benjamin Mahler wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 906 (patched)
> > <https://reviews.apache.org/r/58872/diff/1/?file=1703827#file1703827line906>
> >
> > Should this be called "update"? It looks to me like it's responsible for redirecting.
> >
> > Also, like Tomasz pointed out, I can't really tell what the if condition below does to fix the problem. Would you mind spelling out the problem and the fix in the description?
Fixed, thank you.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review173525
-----------------------------------------------------------
On May 3, 2017, 4:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated May 3, 2017, 4:15 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review173525
-----------------------------------------------------------
Thanks for tackling this!
src/webui/master/static/js/controllers.js
Lines 906 (patched)
<https://reviews.apache.org/r/58872/#comment246512>
Should this be called "update"? It looks to me like it's responsible for redirecting.
Also, like Tomasz pointed out, I can't really tell what the if condition below does to fix the problem. Would you mind spelling out the problem and the fix in the description?
- Benjamin Mahler
On April 29, 2017, 6:22 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated April 29, 2017, 6:22 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensured sandbox URI request reroute after fetched `$scope.state`.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by Tomasz Janiszewski <ja...@gmail.com>.
> On Maj 1, 2017, 8:45 po poĊudniu, Tomasz Janiszewski wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 1024 (patched)
> > <https://reviews.apache.org/r/58872/diff/1/?file=1703827#file1703827line1024>
> >
> > It's not obious why we remove update listener on route change start. Can you add a comment with clarification?
>
> haosdent huang wrote:
> It is because we no need to call this again if route change, so we remove it here. We do it in almost places which depends on `$scope.state`.
>
> ```
> var removeListener = $scope.$on('state_updated', update);
> $scope.$on('$routeChangeStart', removeListener);
> ```
>
> Do you think we need to add comment about this here?
No that's fine. Just spoted it's a template.
- Tomasz
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review173504
-----------------------------------------------------------
On Maj 3, 2017, 4:15 rano, haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated Maj 3, 2017, 4:15 rano)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by haosdent huang <ha...@apache.org>.
> On May 1, 2017, 8:45 p.m., Tomasz Janiszewski wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 1019 (patched)
> > <https://reviews.apache.org/r/58872/diff/1/?file=1703827#file1703827line1019>
> >
> > Please add a comment for this condition with information why it can happen. The comment could be the same as previously used:
> >
> > > When navigating directly to this page, e.g. pasting the URL into the browser
Thank you very much, have updated.
> On May 1, 2017, 8:45 p.m., Tomasz Janiszewski wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 1024 (patched)
> > <https://reviews.apache.org/r/58872/diff/1/?file=1703827#file1703827line1024>
> >
> > It's not obious why we remove update listener on route change start. Can you add a comment with clarification?
It is because we no need to call this again if route change, so we remove it here. We do it in almost places which depends on `$scope.state`.
```
var removeListener = $scope.$on('state_updated', update);
$scope.$on('$routeChangeStart', removeListener);
```
Do you think we need to add comment about this here?
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review173504
-----------------------------------------------------------
On May 3, 2017, 4:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated May 3, 2017, 4:15 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by Tomasz Janiszewski <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review173504
-----------------------------------------------------------
Fix it, then Ship it!
I tested it and it fixes the issue. I think code need some comments, it's simple but not obvious.
src/webui/master/static/js/controllers.js
Lines 1019 (patched)
<https://reviews.apache.org/r/58872/#comment246494>
Please add a comment for this condition with information why it can happen. The comment could be the same as previously used:
> When navigating directly to this page, e.g. pasting the URL into the browser
src/webui/master/static/js/controllers.js
Lines 1024 (patched)
<https://reviews.apache.org/r/58872/#comment246496>
It's not obious why we remove update listener on route change start. Can you add a comment with clarification?
- Tomasz Janiszewski
On Kwi 29, 2017, 6:22 po po\u0142udniu, haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated Kwi 29, 2017, 6:22 po po\u0142udniu)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensured sandbox URI request reroute after fetched `$scope.state`.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review174439
-----------------------------------------------------------
Patch looks great!
Reviews applied: [58872]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 3, 2017, 4:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated May 3, 2017, 4:15 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by haosdent huang <ha...@apache.org>.
> On May 9, 2017, 8:54 p.m., Benjamin Mahler wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 1016-1017 (patched)
> > <https://reviews.apache.org/r/58872/diff/2/?file=1706418#file1706418line1026>
> >
> > I'm a bit confused as to why we would want to set up the listener if we already called reroute(). Does this belong in the else case?
> >
> > ```
> > if ($scope.state) {
> > reroute();
> > } else {
> > var removeListener = $scope.$on('state_updated', reroute);
> > $scope.$on('$routeChangeStart', removeListener);
> > }
> > ```
> >
> > Also, why is it safe to call reroute on every state update? It seems there are a few cases to consider:
> >
> > Case 1: reroute actually routes away from the page, then we're ok because the listener is removed
> >
> > Case 2: reroute calls goBack, we're ok because the listener is removed (there is a separate question we should consider later, which is whether going back is the best thing to do)
> >
> > Case 3: reroute navigates "home" (/) if we get an error response, this is ok since the listener is removed
> >
> > How about adding a comment here that reroute is expected to always route away from the page and so the listener is removed after the first state update?
Thanks @bmahler's detail review. Let me add a comment for it.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review174374
-----------------------------------------------------------
On May 3, 2017, 4:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated May 3, 2017, 4:15 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by haosdent huang <ha...@apache.org>.
> On May 9, 2017, 8:54 p.m., Benjamin Mahler wrote:
> > Before you commit this, it would be great if you could test this (I can't tell how you tested this from the testing done section), e.g.:
> >
> > * Clicking on a link from the top level task page (state should already be loaded)
> > * Navigating directly to a valid URL (state should not be loaded yet)
> > * Navigating directly to an invalid URL (e.g. bad agent ID) (state should not be loaded)
> >
> > To make sure that these all work as you expect.
> Clicking on a link from the top level task page
Works
> Navigating directly to a valid URL (state should not be loaded yet)
Works
> Navigating directly to an invalid URL (e.g. bad agent ID) (state should not be loaded)
Back to last page. It is expected.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review174374
-----------------------------------------------------------
On May 3, 2017, 4:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated May 3, 2017, 4:15 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review174374
-----------------------------------------------------------
Fix it, then Ship it!
Before you commit this, it would be great if you could test this (I can't tell how you tested this from the testing done section), e.g.:
* Clicking on a link from the top level task page (state should already be loaded)
* Navigating directly to a valid URL (state should not be loaded yet)
* Navigating directly to an invalid URL (e.g. bad agent ID) (state should not be loaded)
To make sure that these all work as you expect.
src/webui/master/static/js/controllers.js
Lines 1016-1017 (patched)
<https://reviews.apache.org/r/58872/#comment247501>
I'm a bit confused as to why we would want to set up the listener if we already called reroute(). Does this belong in the else case?
```
if ($scope.state) {
reroute();
} else {
var removeListener = $scope.$on('state_updated', reroute);
$scope.$on('$routeChangeStart', removeListener);
}
```
Also, why is it safe to call reroute on every state update? It seems there are a few cases to consider:
Case 1: reroute actually routes away from the page, then we're ok because the listener is removed
Case 2: reroute calls goBack, we're ok because the listener is removed (there is a separate question we should consider later, which is whether going back is the best thing to do)
Case 3: reroute navigates "home" (/) if we get an error response, this is ok since the listener is removed
How about adding a comment here that reroute is expected to always route away from the page and so the listener is removed after the first state update?
- Benjamin Mahler
On May 3, 2017, 4:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated May 3, 2017, 4:15 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by haosdent huang <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/
-----------------------------------------------------------
(Updated June 20, 2017, 7:51 a.m.)
Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
Changes
-------
Rebase.
Bugs: MESOS-4992
https://issues.apache.org/jira/browse/MESOS-4992
Repository: mesos
Description
-------
Open sandbox link in a new tab would fail since it depends on agents
information loading finish. This patch fixes it by ensuring the agents
information loaded first and then rerouting the sandbox request.
Diffs (updated)
-----
src/webui/master/static/js/controllers.js 1aca25ab6e0856128ebefe5037cbe63430857c99
Diff: https://reviews.apache.org/r/58872/diff/4/
Changes: https://reviews.apache.org/r/58872/diff/3-4/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by haosdent huang <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/
-----------------------------------------------------------
(Updated June 20, 2017, 7:44 a.m.)
Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
Changes
-------
Rebase.
Bugs: MESOS-4992
https://issues.apache.org/jira/browse/MESOS-4992
Repository: mesos
Description
-------
Open sandbox link in a new tab would fail since it depends on agents
information loading finish. This patch fixes it by ensuring the agents
information loaded first and then rerouting the sandbox request.
Diffs (updated)
-----
src/checks/checker_process.hpp 5d0c36dd3e1edf68527fc109d8c0708f09b7e12f
src/checks/checker_process.cpp 66f5452bb5f9c7f45819c221be7b098e5999b7a7
src/common/resources.cpp ef54678d02c03460dcc01779bf5cd974f486f054
src/common/resources_utils.hpp d7d439afdfac92dae51cdd82fc95961f5bfb5c04
src/common/resources_utils.cpp 19c89bffcc62224c6a6b195f71caf729b79ed592
src/master/validation.cpp bebfe53210b923e94e80f91ced8ea17818bbc807
src/v1/resources.cpp c1b6b8a4e5aaa7a45aa59f1808432b831a1c72a6
src/webui/master/static/js/controllers.js 1aca25ab6e0856128ebefe5037cbe63430857c99
Diff: https://reviews.apache.org/r/58872/diff/3/
Changes: https://reviews.apache.org/r/58872/diff/2-3/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by haosdent huang <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/
-----------------------------------------------------------
(Updated May 3, 2017, 4:15 a.m.)
Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
Changes
-------
Address @bmahler, @janisz comments.
Bugs: MESOS-4992
https://issues.apache.org/jira/browse/MESOS-4992
Repository: mesos
Description (updated)
-------
Open sandbox link in a new tab would fail since it depends on agents
information loading finish. This patch fixes it by ensuring the agents
information loaded first and then rerouting the sandbox request.
Diffs (updated)
-----
src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
Diff: https://reviews.apache.org/r/58872/diff/2/
Changes: https://reviews.apache.org/r/58872/diff/1-2/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 58872: Ensured sandbox URI request reroute after
fetched `$scope.state`.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58872/#review173431
-----------------------------------------------------------
Patch looks great!
Reviews applied: [58872]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On April 29, 2017, 6:22 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> -----------------------------------------------------------
>
> (Updated April 29, 2017, 6:22 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
>
>
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Ensured sandbox URI request reroute after fetched `$scope.state`.
>
>
> Diffs
> -----
>
> src/webui/master/static/js/controllers.js a021962573d452de1581e6a7717016eac7d0cd85
>
>
> Diff: https://reviews.apache.org/r/58872/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>