OmnipotentSnitch: added a rough approximation of a possible implementation.

OmnipotentSnitch: added a rough approximation of a possible implementation.

[tonythomas, 2017-10-07 10:30:24]

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...
File src/java/org/apache/cassandra/locator/OmnipotentSnitch.java (right):

https://codereview.appspot.com/329390043/diff/1/src/java/org/apache/cassandra...
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...
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...
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)

[seregibenjamin, 2017-10-07 11:23:44]

*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...:
}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...:
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...:
{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/
>

[seregibenjamin, 2017-10-07 11:32:04]

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...:
> }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...:
> 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...:
> {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/
>>
>
>

[tonythomas, 2017-10-07 11:33:48]

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...:
>> }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...:
>> 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...:
>> {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/
>>>
>>
>>
>

[seregibenjamin, 2017-10-07 11:35:15]

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...:
>>> }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...:
>>> 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...:
>>> {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/
>>>>
>>>
>>>
>>
>