You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Bernd Mathiske <be...@mesosphere.io> on 2014/07/25 02:40:53 UTC
Review Request 23911: multihashmap.getKey() method (useful for fixing
MESOS-947)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23911/
-----------------------------------------------------------
Review request for mesos.
Bugs: MESOS-947
https://issues.apache.org/jira/browse/MESOS-947
Repository: mesos-git
Description
-------
Small extension of stout that is useful for fixing MESOS-947, but also in general. adding a getKey() method to multihashmap, which finds a key given a value in the map.
A dependent Mesos patch fixing MESOS-947 will follow promptly.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp ecda6a9
Diff: https://reviews.apache.org/r/23911/diff/
Testing
-------
The method behaves as expected in the unit test for MESOS-947. I did not create stout unit tests for it. (There is no precedence for testing multihashmap and this particular piece of code is very straightforward.)
Thanks,
Bernd Mathiske
Re: Review Request 23911: multihashmap.getKey() method (useful for fixing
MESOS-947)
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23911/#review48804
-----------------------------------------------------------
Patch looks great!
Reviews applied: [23911]
All tests passed.
- Mesos ReviewBot
On July 25, 2014, 12:40 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23911/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 12:40 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-947
> https://issues.apache.org/jira/browse/MESOS-947
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small extension of stout that is useful for fixing MESOS-947, but also in general. adding a getKey() method to multihashmap, which finds a key given a value in the map.
>
> A dependent Mesos patch fixing MESOS-947 will follow promptly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp ecda6a9
>
> Diff: https://reviews.apache.org/r/23911/diff/
>
>
> Testing
> -------
>
> The method behaves as expected in the unit test for MESOS-947. I did not create stout unit tests for it. (There is no precedence for testing multihashmap and this particular piece of code is very straightforward.)
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 23911: multihashmap.getKey() method (useful for fixing
MESOS-947)
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23911/#review49551
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On July 28, 2014, 8:22 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23911/
> -----------------------------------------------------------
>
> (Updated July 28, 2014, 8:22 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-947
> https://issues.apache.org/jira/browse/MESOS-947
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small extension of stout that is useful for fixing MESOS-947, but also in general. adding a getKey() method to multihashmap, which finds a key given a value in the map.
>
> A dependent Mesos patch fixing MESOS-947 will follow promptly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp ecda6a941077b3e4f9585674b0b5d00d35c5aa1a
>
> Diff: https://reviews.apache.org/r/23911/diff/
>
>
> Testing
> -------
>
> The method behaves as expected in the unit test for MESOS-947. I did not create stout unit tests for it. (There is no precedence for testing multihashmap and this particular piece of code is very straightforward.)
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 23911: multihashmap.getKey() method (useful for fixing
MESOS-947)
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23911/
-----------------------------------------------------------
(Updated July 28, 2014, 1:22 p.m.)
Review request for mesos.
Changes
-------
Fixed issue. Thx, Ben M, good call. I also like the more general method better now. It is very unlikely that there will ever be tasks with the identical ID that are set up to run with multiple executors at the same time, but let's program defensively anyway.
Bugs: MESOS-947
https://issues.apache.org/jira/browse/MESOS-947
Repository: mesos-git
Description
-------
Small extension of stout that is useful for fixing MESOS-947, but also in general. adding a getKey() method to multihashmap, which finds a key given a value in the map.
A dependent Mesos patch fixing MESOS-947 will follow promptly.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp ecda6a941077b3e4f9585674b0b5d00d35c5aa1a
Diff: https://reviews.apache.org/r/23911/diff/
Testing
-------
The method behaves as expected in the unit test for MESOS-947. I did not create stout unit tests for it. (There is no precedence for testing multihashmap and this particular piece of code is very straightforward.)
Thanks,
Bernd Mathiske
Re: Review Request 23911: multihashmap.getKey() method (useful for fixing
MESOS-947)
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On July 24, 2014, 5:51 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp, line 42
> > <https://reviews.apache.org/r/23911/diff/1/?file=641620#file641620line42>
> >
> > Aren't there potentially multiple keys for a particular value?
> >
> > i.e. Option<hashset<K> > getKeys(const V& value) const;
> >
> > Or should the calling code just iterate through the map for now to find the first key?
>
> Ben Mahler wrote:
> Sorry, the Option was unnecessary. :)
I could add the method that returns all keys for a given value, but I do not have a use case for it.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23911/#review48705
-----------------------------------------------------------
On July 24, 2014, 5:40 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23911/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 5:40 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-947
> https://issues.apache.org/jira/browse/MESOS-947
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small extension of stout that is useful for fixing MESOS-947, but also in general. adding a getKey() method to multihashmap, which finds a key given a value in the map.
>
> A dependent Mesos patch fixing MESOS-947 will follow promptly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp ecda6a9
>
> Diff: https://reviews.apache.org/r/23911/diff/
>
>
> Testing
> -------
>
> The method behaves as expected in the unit test for MESOS-947. I did not create stout unit tests for it. (There is no precedence for testing multihashmap and this particular piece of code is very straightforward.)
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 23911: multihashmap.getKey() method (useful for fixing
MESOS-947)
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On July 24, 2014, 5:51 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp, line 42
> > <https://reviews.apache.org/r/23911/diff/1/?file=641620#file641620line42>
> >
> > Aren't there potentially multiple keys for a particular value?
> >
> > i.e. Option<hashset<K> > getKeys(const V& value) const;
> >
> > Or should the calling code just iterate through the map for now to find the first key?
>
> Ben Mahler wrote:
> Sorry, the Option was unnecessary. :)
>
> Bernd Mathiske wrote:
> I could add the method that returns all keys for a given value, but I do not have a use case for it.
>
> Ben Mahler wrote:
> My concern is that it seems to be tailored to your particular use-case, where you happen to have unique values per key. But this is not an inherent property of multimap.
>
> For your use-case getKeys() is equivalent:
>
> getKey getKeys
> None <-> Empty set
> Some K <-> Non-empty set (use *iterator.begin() to obtain the only key)
>
> Or you could just iterate the map since there only appears to be one use case so far?
OK, I will change it to getKeys(), which returns all keys. This will make my code more complex and slower, but it does not matter here. It only gets executed when a task has been lost or killed between runTask() and _runTask().
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23911/#review48705
-----------------------------------------------------------
On July 24, 2014, 5:40 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23911/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 5:40 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-947
> https://issues.apache.org/jira/browse/MESOS-947
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small extension of stout that is useful for fixing MESOS-947, but also in general. adding a getKey() method to multihashmap, which finds a key given a value in the map.
>
> A dependent Mesos patch fixing MESOS-947 will follow promptly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp ecda6a9
>
> Diff: https://reviews.apache.org/r/23911/diff/
>
>
> Testing
> -------
>
> The method behaves as expected in the unit test for MESOS-947. I did not create stout unit tests for it. (There is no precedence for testing multihashmap and this particular piece of code is very straightforward.)
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 23911: multihashmap.getKey() method (useful for fixing
MESOS-947)
Posted by Ben Mahler <be...@gmail.com>.
> On July 25, 2014, 12:51 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp, line 42
> > <https://reviews.apache.org/r/23911/diff/1/?file=641620#file641620line42>
> >
> > Aren't there potentially multiple keys for a particular value?
> >
> > i.e. Option<hashset<K> > getKeys(const V& value) const;
> >
> > Or should the calling code just iterate through the map for now to find the first key?
Sorry, the Option was unnecessary. :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23911/#review48705
-----------------------------------------------------------
On July 25, 2014, 12:40 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23911/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 12:40 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-947
> https://issues.apache.org/jira/browse/MESOS-947
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small extension of stout that is useful for fixing MESOS-947, but also in general. adding a getKey() method to multihashmap, which finds a key given a value in the map.
>
> A dependent Mesos patch fixing MESOS-947 will follow promptly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp ecda6a9
>
> Diff: https://reviews.apache.org/r/23911/diff/
>
>
> Testing
> -------
>
> The method behaves as expected in the unit test for MESOS-947. I did not create stout unit tests for it. (There is no precedence for testing multihashmap and this particular piece of code is very straightforward.)
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 23911: multihashmap.getKey() method (useful for fixing
MESOS-947)
Posted by Ben Mahler <be...@gmail.com>.
> On July 25, 2014, 12:51 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp, line 42
> > <https://reviews.apache.org/r/23911/diff/1/?file=641620#file641620line42>
> >
> > Aren't there potentially multiple keys for a particular value?
> >
> > i.e. Option<hashset<K> > getKeys(const V& value) const;
> >
> > Or should the calling code just iterate through the map for now to find the first key?
>
> Ben Mahler wrote:
> Sorry, the Option was unnecessary. :)
>
> Bernd Mathiske wrote:
> I could add the method that returns all keys for a given value, but I do not have a use case for it.
My concern is that it seems to be tailored to your particular use-case, where you happen to have unique values per key. But this is not an inherent property of multimap.
For your use-case getKeys() is equivalent:
getKey getKeys
None <-> Empty set
Some K <-> Non-empty set (use *iterator.begin() to obtain the only key)
Or you could just iterate the map since there only appears to be one use case so far?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23911/#review48705
-----------------------------------------------------------
On July 25, 2014, 12:40 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23911/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 12:40 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-947
> https://issues.apache.org/jira/browse/MESOS-947
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small extension of stout that is useful for fixing MESOS-947, but also in general. adding a getKey() method to multihashmap, which finds a key given a value in the map.
>
> A dependent Mesos patch fixing MESOS-947 will follow promptly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp ecda6a9
>
> Diff: https://reviews.apache.org/r/23911/diff/
>
>
> Testing
> -------
>
> The method behaves as expected in the unit test for MESOS-947. I did not create stout unit tests for it. (There is no precedence for testing multihashmap and this particular piece of code is very straightforward.)
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 23911: multihashmap.getKey() method (useful for fixing
MESOS-947)
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23911/#review48705
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp
<https://reviews.apache.org/r/23911/#comment85467>
Aren't there potentially multiple keys for a particular value?
i.e. Option<hashset<K> > getKeys(const V& value) const;
Or should the calling code just iterate through the map for now to find the first key?
- Ben Mahler
On July 25, 2014, 12:40 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23911/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 12:40 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-947
> https://issues.apache.org/jira/browse/MESOS-947
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Small extension of stout that is useful for fixing MESOS-947, but also in general. adding a getKey() method to multihashmap, which finds a key given a value in the map.
>
> A dependent Mesos patch fixing MESOS-947 will follow promptly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp ecda6a9
>
> Diff: https://reviews.apache.org/r/23911/diff/
>
>
> Testing
> -------
>
> The method behaves as expected in the unit test for MESOS-947. I did not create stout unit tests for it. (There is no precedence for testing multihashmap and this particular piece of code is very straightforward.)
>
>
> Thanks,
>
> Bernd Mathiske
>
>