Brandon Williams commented on CASSANDRA-12791:

Yep, that's totally fine by me, I've never seen a case of this being 
misreported, and reporting it, but knowing if it was due to cross-node is 
fairly critical, troubleshooting this scenario before we distinguished was 
pretty hellacious.

> MessageIn logic to determine if the message is cross-node is wrong
> ------------------------------------------------------------------
>                 Key: CASSANDRA-12791
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-12791
>             Project: Cassandra
>          Issue Type: Bug
>            Reporter: Sylvain Lebresne
>            Assignee: Sylvain Lebresne
>            Priority: Minor
> {{MessageIn}} has the following code to read the 'creation time' of the 
> message on the receiving side:
> {noformat}
> public static ConstructionTime readTimestamp(InetAddress from, DataInputPlus 
> input, long timestamp) throws IOException
> {
>     // make sure to readInt, even if cross_node_to is not enabled
>     int partial = input.readInt();
>     long crossNodeTimestamp = (timestamp & 0xFFFFFFFF00000000L) | (((partial 
> & 0xFFFFFFFFL) << 2) >> 2);
>     if (timestamp > crossNodeTimestamp)
>     {
>         MessagingService.instance().metrics.addTimeTaken(from, timestamp - 
> crossNodeTimestamp);
>     }
>     if(DatabaseDescriptor.hasCrossNodeTimeout())
>     {
>         return new ConstructionTime(crossNodeTimestamp, timestamp != 
> crossNodeTimestamp);
>     }
>     else
>     {
>         return new ConstructionTime();
>     }
> }
> {noformat}
> where {{timestamp}} is really the local time on the receiving node when 
> calling that method.
> The incorrect part, I believe, is the {{timestamp != crossNodeTimestamp}} 
> used to set the {{isCrossNode}} field of {{ConstructionTime}}. A first 
> problem is that this will basically always be {{true}}: for it to be 
> {{false}}, we'd need the low-bytes of the timestamp taken on the sending node 
> to coincide exactly with the ones taken on the receiving side, which is 
> _very_ unlikely. It is also a relatively meaningless test: having that test 
> be {{false}} basically means the lack of clock sync between the 2 nodes is 
> exactly the time the 2 calls to {{System.currentTimeMillis()}} (on sender and 
> receiver), which is definitively not what we care about.
> What the result of this test is used for is to determine if the message was 
> crossNode or local. It's used to increment different metrics (we separate 
> metric local versus crossNode dropped messages) in {{MessagingService}} for 
> instance. And that's where this is kind of a bug: not only the {{timestamp != 
> crossNodeTimestamp}}, but if {{DatabaseDescriptor.hasCrossNodeTimeout()}}, we 
> *always* have this {{isCrossNode}} false, which means we'll never increment 
> the "cross-node dropped messages" metric, which is imo unexpected.
> That is, it is true that if {{DatabaseDescriptor.hasCrossNodeTimeout() == 
> false}}, then we end using the receiver side timestamp to timeout messages, 
> and so you end up only dropping messages that timeout locally. And _in that 
> sense_, always incrementing the "locally" dropped messages metric is not 
> completely illogical. But I doubt most users are aware of those pretty 
> specific nuance when looking at the related metrics, and I'm relatively sure 
> users expect a metrics named {{droppedCrossNodeTimeout}} to actually count 
> cross-node messages by default (keep in mind that 
> {{DatabaseDescriptor.hasCrossNodeTimeout()}} is actually false by default).
> Anyway, to sum it up I suggest that the following change should be done:
> # the {{timestamp != crossNodeTimestamp}} test is definitively not what we 
> want. We should at a minimum just replace it to {{true}} as that's basically 
> what it ends up being except for very rare and arguably random cases.
> # given how the {{ConstructionTime.isCrossNode}} is used, I suggest that we 
> really want it to mean if the message has shipped cross-node, not just be a 
> synonymous of {{DatabaseDescriptor.hasCrossNodeTimeout()}}. It should be 
> whether the message shipped cross-node, i.e. whether {{from == 
> BroadcastAdress()}} or not.

This message was sent by Atlassian JIRA

Reply via email to