You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Avinash sridharan <av...@mesosphere.io> on 2015/11/26 07:52:25 UTC
Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to avoid
errors due to double precision errors.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/
-----------------------------------------------------------
Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
Bugs: MESOS-3552
https://issues.apache.org/jira/browse/MESOS-3552
Repository: mesos
Description
-------
A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
Diffs
-----
src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
Diff: https://reviews.apache.org/r/40730/diff/
Testing
-------
Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
Thanks,
Avinash sridharan
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108134
-----------------------------------------------------------
Sorry for multiple shards instead of a single review.
src/common/resources.cpp (lines 43 - 44)
<https://reviews.apache.org/r/40730/#comment167491>
Blank line, please.
- Alexander Rukletsov
On Nov. 26, 2015, 6:52 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2015, 6:52 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Nov. 26, 2015, 6:20 a.m., Bernd Mathiske wrote:
> > Ship It!
An addendum intended to address the issues below is available here:
https://reviews.apache.org/r/40767/
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108129
-----------------------------------------------------------
On Nov. 25, 2015, 10:52 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2015, 10:52 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108129
-----------------------------------------------------------
Ship it!
Ship It!
- Bernd Mathiske
On Nov. 25, 2015, 10:52 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2015, 10:52 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Nov. 26, 2015, 7:14 a.m., Neil Conway wrote:
> > src/common/resources.cpp, line 951
> > <https://reviews.apache.org/r/40730/diff/1/?file=1147236#file1147236line951>
> >
> > This definitely needs an explanatory comment.
I'll propose a patch that addresses these issues.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108140
-----------------------------------------------------------
On Nov. 25, 2015, 10:52 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2015, 10:52 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Mandeep Chadha <ma...@yahoo.com>.
> On Nov. 26, 2015, 3:14 p.m., Neil Conway wrote:
> > src/common/resources.cpp, line 951
> > <https://reviews.apache.org/r/40730/diff/1/?file=1147236#file1147236line951>
> >
> > This definitely needs an explanatory comment.
>
> Bernd Mathiske wrote:
> I'll propose a patch that addresses these issues.
What if results or cpus returns isNone or one of them is isNone ( Option ) , then get() is going to crash. CHECK_NEAR() does the same thing as the previous review request and this is the history of previous review request :
"CHECK_DOUBLE_EQ will also fail.
F0930 18:15:38.169140 26984 resources.cpp:874] Check failed: (result.cpus().get()) >= (cpus().get())-0.000000000000001L (24 vs. 24) Check failure stack trace: F0930 18:15:38.169322 26991 resources.cpp:874] Check failed: (result.cpus().get()) >= (cpus().get())-0.000000000000001L (24 vs. 24) Check failure stack trace:
CHECK_NEAR() is the right Macro to use.
But we also need to check (isNone() and isNone()) equality and hence the present implementation. I think the long term fix is to wrap double into Double and have the opeartor== to the right thing.
"
Using CHECK_NEAR() is fine but not checking other state of Option<> may lead to crash and is not complete.
- Mandeep
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108140
-----------------------------------------------------------
On Nov. 26, 2015, 6:52 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2015, 6:52 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108140
-----------------------------------------------------------
src/common/resources.cpp (line 949)
<https://reviews.apache.org/r/40730/#comment167492>
This definitely needs an explanatory comment.
- Neil Conway
On Nov. 26, 2015, 6:52 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2015, 6:52 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108136
-----------------------------------------------------------
Ship it!
Ship It!
- Klaus Ma
On Nov. 26, 2015, 2:52 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2015, 2:52 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Nov. 26, 2015, 6:43 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, line 35
> > <https://reviews.apache.org/r/40730/diff/1/?file=1147236#file1147236line35>
> >
> > Is it fine to include files from "master/" in "common/*"?
Addressed in https://reviews.apache.org/r/40767/
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108132
-----------------------------------------------------------
On Nov. 25, 2015, 10:52 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2015, 10:52 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108132
-----------------------------------------------------------
src/common/resources.cpp (line 35)
<https://reviews.apache.org/r/40730/#comment167488>
Is it fine to include files from "master/" in "common/*"?
- Alexander Rukletsov
On Nov. 26, 2015, 6:52 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2015, 6:52 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Nov. 26, 2015, 6:27 a.m., Till Toenshoff wrote:
> > src/common/resources.cpp, line 947
> > <https://reviews.apache.org/r/40730/diff/1/?file=1147236#file1147236line947>
> >
> > Why introducing the leading space after the bracket?
> >
> > s/CHECK( /CHECK(/g
I will fix this when committing.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108130
-----------------------------------------------------------
On Nov. 25, 2015, 10:52 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2015, 10:52 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108130
-----------------------------------------------------------
Ship it!
Thanks a bunch for coming up with this short-term-fix, Avinash!
src/common/resources.cpp (line 946)
<https://reviews.apache.org/r/40730/#comment167486>
Why introducing the leading space after the bracket?
s/CHECK( /CHECK(/g
- Till Toenshoff
On Nov. 26, 2015, 6:52 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2015, 6:52 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 40730: Fixing MESOS-3552 by using CHECK_NEAR to
avoid errors due to double precision errors.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40730/#review108133
-----------------------------------------------------------
src/common/resources.cpp
<https://reviews.apache.org/r/40730/#comment167489>
Is this line removed?
src/common/resources.cpp (lines 948 - 949)
<https://reviews.apache.org/r/40730/#comment167490>
We tend to insert a blank line if the previous statement spans multiple lines. How about swapping the statements?
- Alexander Rukletsov
On Nov. 26, 2015, 6:52 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40730/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2015, 6:52 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Klaus Ma, and Neil Conway.
>
>
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A version of this fix was already proposed by Mandeep Chadha (@mchadha) and submitted to the reivew board https://reviews.apache.org/r/39056/ . So the main credit should go to him for this fix.
>
>
> Diffs
> -----
>
> src/common/resources.cpp b4abf5405039d7d0a5028ccf034ad2e9623d064c
>
> Diff: https://reviews.apache.org/r/40730/diff/
>
>
> Testing
> -------
>
> Added Mandeep's test case to GTEST https://reviews.apache.org/r/39056/ and enabled an existing precision test (https://reviews.apache.org/r/40732/) to test this fix.
>
>
> Thanks,
>
> Avinash sridharan
>
>