Code review - Issue 329390043: OmnipotentSnitch: added a rough approximation of a possible implementation.https://codereview.appspot.com/2017-10-13T13:01:46+00:00rietveld
Message from unknown
2017-10-06T23:47:05+00:00seregibenjaminurn:md5:0ee6d1133836c12cfec7f2d729c9a789
Message from seregibenjamin@gmail.com
2017-10-06T23:47:22+00:00seregibenjaminurn:md5:dd0c99fbc26dc6ac83302b9f85b046f4
Message from 01tonythomas@gmail.com
2017-10-07T10:30:24+00:00tonythomasurn:md5:e9d2fecc2543158db8fe2fc3318d20d6
Great work man. Would really appreciate making few details bit more verbose so that non-java people in this list can explain later.
https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java
File src/java/org/apache/cassandra/locator/OmnipotentSnitch.java (right):
https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode81
src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:81: }
I dont know about the internals, but is the python program giving latency to the Snitchpoint in the right unit ? Like we are taking it directly from ping, and its in ms.
https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode91
src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:91: public List<InetAddress> getSortedListByProximity(final InetAddress address, Collection<InetAddress> addresses)
Can you document what this do ? Or is it a mandatory implementation (due to inheritance) ?
https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode124
src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:124: {
not a java guy, but I guess what you are trying to do is to sort addresses with your unnamed function "new Comparator<InetAddress>() {}" ? If yes, can we make it a bit more readable, even making it a named function so that people with little know-how of how this thing works (like me) can explain this later ?
Also, if it is, I am shocked to see the `{` on the same indent of `Collections.sort...()` which created this confusion (py effect)
Message from seregibenjamin@gmail.com
2017-10-07T11:23:44+00:00seregibenjaminurn:md5:a5bc2408c23c44aa066616f213666165
*https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode81
<https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode81>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:81:
}I dont know about the internals, but is the python program givinglatency
to the Snitchpoint in the right unit ? Like we are taking itdirectly from
ping, and its in ms.*
Yes, it is in ms, but the unit does not matter as the delays are used
internally to sort the hosts.
*https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode91
<https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode91>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:91:
publicList<InetAddress> getSortedListByProximity(final InetAddress
address,Collection<InetAddress> addresses)Can you document what this do ?
Or is it a mandatory implementation (dueto inheritance) ?*
Yes, it is from AbstractEndpointSnitch. I had to override it since we
cannot allow compareEndpoints() to be called directly from OmnipotentSnitch
(the same issue with DS). This is a design issue in Cassandra (IMHO),
DynamicEndpointSnitch and OmnipotentSnitch should not implement the
abstract class AbstractEndpointSnitch as they work as a wrapper around
other snitches and therefore their functionality is somewhat different.
*https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode124
<https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode124>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:124:
{not a java guy, but I guess what you are trying to do is to sortaddresses
with your unnamed function "new Comparator<InetAddress>() {}"? If yes, can
we make it a bit more readable, even making it a namedfunction so that
people with little know-how of how this thing works(like me) can explain
this later ?Also, if it is, I am shocked to see the `{` on the same indent
of`Collections.sort...()` which created this confusion (py effect)*
I am not a Java-guy either. :) We can do but in this case, it is good to
use a lambda.
Thanks for the review.
Ben
On 7 October 2017 at 12:30, <01tonythomas@gmail.com> wrote:
> Great work man. Would really appreciate making few details bit more
> verbose so that non-java people in this list can explain later.
>
>
> https://codereview.appspot.com/329390043/diff/1/src/java/org
> /apache/cassandra/locator/OmnipotentSnitch.java
> File src/java/org/apache/cassandra/locator/OmnipotentSnitch.java
> (right):
>
> https://codereview.appspot.com/329390043/diff/1/src/java/org
> /apache/cassandra/locator/OmnipotentSnitch.java#newcode81
> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:81: }
> I dont know about the internals, but is the python program giving
> latency to the Snitchpoint in the right unit ? Like we are taking it
> directly from ping, and its in ms.
>
> https://codereview.appspot.com/329390043/diff/1/src/java/org
> /apache/cassandra/locator/OmnipotentSnitch.java#newcode91
> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:91: public
> List<InetAddress> getSortedListByProximity(final InetAddress address,
> Collection<InetAddress> addresses)
> Can you document what this do ? Or is it a mandatory implementation (due
> to inheritance) ?
>
> https://codereview.appspot.com/329390043/diff/1/src/java/org
> /apache/cassandra/locator/OmnipotentSnitch.java#newcode124
> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:124: {
> not a java guy, but I guess what you are trying to do is to sort
> addresses with your unnamed function "new Comparator<InetAddress>() {}"
> ? If yes, can we make it a bit more readable, even making it a named
> function so that people with little know-how of how this thing works
> (like me) can explain this later ?
>
> Also, if it is, I am shocked to see the `{` on the same indent of
> `Collections.sort...()` which created this confusion (py effect)
>
> https://codereview.appspot.com/329390043/
>
Message from seregibenjamin@gmail.com
2017-10-07T11:32:04+00:00seregibenjaminurn:md5:8e17eae0fe7802887a01fd8b2979a15c
I would like to correct my last comment. It is not a lambda but a
Comparator instance with an overridden compare() function.
On 7 October 2017 at 13:23, Benjámin Seregi <seregibenjamin@gmail.com>
wrote:
>
>
>
>
> *https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode81
> <https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode81>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:81:
> }I dont know about the internals, but is the python program givinglatency
> to the Snitchpoint in the right unit ? Like we are taking itdirectly from
> ping, and its in ms.*
>
> Yes, it is in ms, but the unit does not matter as the delays are used
> internally to sort the hosts.
>
>
>
>
>
>
> *https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode91
> <https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode91>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:91:
> publicList<InetAddress> getSortedListByProximity(final InetAddress
> address,Collection<InetAddress> addresses)Can you document what this do ?
> Or is it a mandatory implementation (dueto inheritance) ?*
>
> Yes, it is from AbstractEndpointSnitch. I had to override it since we
> cannot allow compareEndpoints() to be called directly from OmnipotentSnitch
> (the same issue with DS). This is a design issue in Cassandra (IMHO),
> DynamicEndpointSnitch and OmnipotentSnitch should not implement the
> abstract class AbstractEndpointSnitch as they work as a wrapper around
> other snitches and therefore their functionality is somewhat different.
>
>
>
>
>
>
>
>
>
>
> *https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode124
> <https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode124>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:124:
> {not a java guy, but I guess what you are trying to do is to sortaddresses
> with your unnamed function "new Comparator<InetAddress>() {}"? If yes, can
> we make it a bit more readable, even making it a namedfunction so that
> people with little know-how of how this thing works(like me) can explain
> this later ?Also, if it is, I am shocked to see the `{` on the same indent
> of`Collections.sort...()` which created this confusion (py effect)*
>
> I am not a Java-guy either. :) We can do but in this case, it is good to
> use a lambda.
>
> Thanks for the review.
>
> Ben
>
> On 7 October 2017 at 12:30, <01tonythomas@gmail.com> wrote:
>
>> Great work man. Would really appreciate making few details bit more
>> verbose so that non-java people in this list can explain later.
>>
>>
>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>> /apache/cassandra/locator/OmnipotentSnitch.java
>> File src/java/org/apache/cassandra/locator/OmnipotentSnitch.java
>> (right):
>>
>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>> /apache/cassandra/locator/OmnipotentSnitch.java#newcode81
>> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:81: }
>> I dont know about the internals, but is the python program giving
>> latency to the Snitchpoint in the right unit ? Like we are taking it
>> directly from ping, and its in ms.
>>
>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>> /apache/cassandra/locator/OmnipotentSnitch.java#newcode91
>> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:91: public
>> List<InetAddress> getSortedListByProximity(final InetAddress address,
>> Collection<InetAddress> addresses)
>> Can you document what this do ? Or is it a mandatory implementation (due
>> to inheritance) ?
>>
>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>> /apache/cassandra/locator/OmnipotentSnitch.java#newcode124
>> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:124: {
>> not a java guy, but I guess what you are trying to do is to sort
>> addresses with your unnamed function "new Comparator<InetAddress>() {}"
>> ? If yes, can we make it a bit more readable, even making it a named
>> function so that people with little know-how of how this thing works
>> (like me) can explain this later ?
>>
>> Also, if it is, I am shocked to see the `{` on the same indent of
>> `Collections.sort...()` which created this confusion (py effect)
>>
>> https://codereview.appspot.com/329390043/
>>
>
>
Message from 01tonythomas@gmail.com
2017-10-07T11:33:48+00:00tonythomasurn:md5:ac30e8107cb8c7d37a990869fc49ee7a
Benjamin, can you please reply via the codereview tool so that later when
someone look at it can see your reply to inline comments there ? Else
others might think it was unanswered.
--
Tony Thomas
https://mediawiki.org/wiki/User:01tonythomas
--
On Sat, Oct 7, 2017 at 1:32 PM, Benjámin Seregi <seregibenjamin@gmail.com>
wrote:
> I would like to correct my last comment. It is not a lambda but a
> Comparator instance with an overridden compare() function.
>
> On 7 October 2017 at 13:23, Benjámin Seregi <seregibenjamin@gmail.com>
> wrote:
>
>>
>>
>>
>>
>> *https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode81
>> <https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode81>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:81:
>> }I dont know about the internals, but is the python program givinglatency
>> to the Snitchpoint in the right unit ? Like we are taking itdirectly from
>> ping, and its in ms.*
>>
>> Yes, it is in ms, but the unit does not matter as the delays are used
>> internally to sort the hosts.
>>
>>
>>
>>
>>
>>
>> *https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode91
>> <https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode91>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:91:
>> publicList<InetAddress> getSortedListByProximity(final InetAddress
>> address,Collection<InetAddress> addresses)Can you document what this do ?
>> Or is it a mandatory implementation (dueto inheritance) ?*
>>
>> Yes, it is from AbstractEndpointSnitch. I had to override it since we
>> cannot allow compareEndpoints() to be called directly from OmnipotentSnitch
>> (the same issue with DS). This is a design issue in Cassandra (IMHO),
>> DynamicEndpointSnitch and OmnipotentSnitch should not implement the
>> abstract class AbstractEndpointSnitch as they work as a wrapper around
>> other snitches and therefore their functionality is somewhat different.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> *https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode124
>> <https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode124>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:124:
>> {not a java guy, but I guess what you are trying to do is to sortaddresses
>> with your unnamed function "new Comparator<InetAddress>() {}"? If yes, can
>> we make it a bit more readable, even making it a namedfunction so that
>> people with little know-how of how this thing works(like me) can explain
>> this later ?Also, if it is, I am shocked to see the `{` on the same indent
>> of`Collections.sort...()` which created this confusion (py effect)*
>>
>> I am not a Java-guy either. :) We can do but in this case, it is good to
>> use a lambda.
>>
>> Thanks for the review.
>>
>> Ben
>>
>> On 7 October 2017 at 12:30, <01tonythomas@gmail.com> wrote:
>>
>>> Great work man. Would really appreciate making few details bit more
>>> verbose so that non-java people in this list can explain later.
>>>
>>>
>>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>>> /apache/cassandra/locator/OmnipotentSnitch.java
>>> File src/java/org/apache/cassandra/locator/OmnipotentSnitch.java
>>> (right):
>>>
>>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>>> /apache/cassandra/locator/OmnipotentSnitch.java#newcode81
>>> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:81: }
>>> I dont know about the internals, but is the python program giving
>>> latency to the Snitchpoint in the right unit ? Like we are taking it
>>> directly from ping, and its in ms.
>>>
>>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>>> /apache/cassandra/locator/OmnipotentSnitch.java#newcode91
>>> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:91: public
>>> List<InetAddress> getSortedListByProximity(final InetAddress address,
>>> Collection<InetAddress> addresses)
>>> Can you document what this do ? Or is it a mandatory implementation (due
>>> to inheritance) ?
>>>
>>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>>> /apache/cassandra/locator/OmnipotentSnitch.java#newcode124
>>> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:124: {
>>> not a java guy, but I guess what you are trying to do is to sort
>>> addresses with your unnamed function "new Comparator<InetAddress>() {}"
>>> ? If yes, can we make it a bit more readable, even making it a named
>>> function so that people with little know-how of how this thing works
>>> (like me) can explain this later ?
>>>
>>> Also, if it is, I am shocked to see the `{` on the same indent of
>>> `Collections.sort...()` which created this confusion (py effect)
>>>
>>> https://codereview.appspot.com/329390043/
>>>
>>
>>
>
Message from seregibenjamin@gmail.com
2017-10-07T11:35:15+00:00seregibenjaminurn:md5:b41b7ca3fe97b48362ac3f590657d2aa
Ok, sorry, I will do so.
On 7 October 2017 at 13:33, Tony Thomas <01tonythomas@gmail.com> wrote:
> Benjamin, can you please reply via the codereview tool so that later when
> someone look at it can see your reply to inline comments there ? Else
> others might think it was unanswered.
>
> --
> Tony Thomas
> https://mediawiki.org/wiki/User:01tonythomas
> --
>
> On Sat, Oct 7, 2017 at 1:32 PM, Benjámin Seregi <seregibenjamin@gmail.com>
> wrote:
>
>> I would like to correct my last comment. It is not a lambda but a
>> Comparator instance with an overridden compare() function.
>>
>> On 7 October 2017 at 13:23, Benjámin Seregi <seregibenjamin@gmail.com>
>> wrote:
>>
>>>
>>>
>>>
>>>
>>> *https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode81
>>> <https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode81>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:81:
>>> }I dont know about the internals, but is the python program givinglatency
>>> to the Snitchpoint in the right unit ? Like we are taking itdirectly from
>>> ping, and its in ms.*
>>>
>>> Yes, it is in ms, but the unit does not matter as the delays are used
>>> internally to sort the hosts.
>>>
>>>
>>>
>>>
>>>
>>>
>>> *https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode91
>>> <https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode91>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:91:
>>> publicList<InetAddress> getSortedListByProximity(final InetAddress
>>> address,Collection<InetAddress> addresses)Can you document what this do ?
>>> Or is it a mandatory implementation (dueto inheritance) ?*
>>>
>>> Yes, it is from AbstractEndpointSnitch. I had to override it since we
>>> cannot allow compareEndpoints() to be called directly from OmnipotentSnitch
>>> (the same issue with DS). This is a design issue in Cassandra (IMHO),
>>> DynamicEndpointSnitch and OmnipotentSnitch should not implement the
>>> abstract class AbstractEndpointSnitch as they work as a wrapper around
>>> other snitches and therefore their functionality is somewhat different.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> *https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode124
>>> <https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra/locator/OmnipotentSnitch.java#newcode124>src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:124:
>>> {not a java guy, but I guess what you are trying to do is to sortaddresses
>>> with your unnamed function "new Comparator<InetAddress>() {}"? If yes, can
>>> we make it a bit more readable, even making it a namedfunction so that
>>> people with little know-how of how this thing works(like me) can explain
>>> this later ?Also, if it is, I am shocked to see the `{` on the same indent
>>> of`Collections.sort...()` which created this confusion (py effect)*
>>>
>>> I am not a Java-guy either. :) We can do but in this case, it is good to
>>> use a lambda.
>>>
>>> Thanks for the review.
>>>
>>> Ben
>>>
>>> On 7 October 2017 at 12:30, <01tonythomas@gmail.com> wrote:
>>>
>>>> Great work man. Would really appreciate making few details bit more
>>>> verbose so that non-java people in this list can explain later.
>>>>
>>>>
>>>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>>>> /apache/cassandra/locator/OmnipotentSnitch.java
>>>> File src/java/org/apache/cassandra/locator/OmnipotentSnitch.java
>>>> (right):
>>>>
>>>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>>>> /apache/cassandra/locator/OmnipotentSnitch.java#newcode81
>>>> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:81: }
>>>> I dont know about the internals, but is the python program giving
>>>> latency to the Snitchpoint in the right unit ? Like we are taking it
>>>> directly from ping, and its in ms.
>>>>
>>>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>>>> /apache/cassandra/locator/OmnipotentSnitch.java#newcode91
>>>> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:91: public
>>>> List<InetAddress> getSortedListByProximity(final InetAddress address,
>>>> Collection<InetAddress> addresses)
>>>> Can you document what this do ? Or is it a mandatory implementation (due
>>>> to inheritance) ?
>>>>
>>>> https://codereview.appspot.com/329390043/diff/1/src/java/org
>>>> /apache/cassandra/locator/OmnipotentSnitch.java#newcode124
>>>> src/java/org/apache/cassandra/locator/OmnipotentSnitch.java:124: {
>>>> not a java guy, but I guess what you are trying to do is to sort
>>>> addresses with your unnamed function "new Comparator<InetAddress>() {}"
>>>> ? If yes, can we make it a bit more readable, even making it a named
>>>> function so that people with little know-how of how this thing works
>>>> (like me) can explain this later ?
>>>>
>>>> Also, if it is, I am shocked to see the `{` on the same indent of
>>>> `Collections.sort...()` which created this confusion (py effect)
>>>>
>>>> https://codereview.appspot.com/329390043/
>>>>
>>>
>>>
>>
>
Message from unknown
2017-10-10T14:46:46+00:00seregibenjaminurn:md5:97cac4a835e2bf701a84732662a8bdc3
Message from unknown
2017-10-10T21:24:39+00:00seregibenjaminurn:md5:53ffec84933a88968ecd7aede88368c4
Message from unknown
2017-10-10T21:57:54+00:00seregibenjaminurn:md5:098d5f40be4a734d066ff20ccd8f4544
Message from seregibenjamin@gmail.com
2017-10-10T22:09:14+00:00seregibenjaminurn:md5:4d1e6b7d3e177e374a2e72ee39cc1bb7
Message from unknown
2017-10-13T12:08:36+00:00seregibenjaminurn:md5:9445483c3d898facd113f660da8d753d
Message from seregibenjamin@gmail.com
2017-10-13T13:01:46+00:00seregibenjaminurn:md5:3cce01b8a1109485f7dc2e65a567fb7b