[jira] [Commented] (CASSANDRA-12791) MessageIn logic to determine if the message is cross-node is wrong
[
https://issues.apache.org/jira/browse/CASSANDRA-12791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15631199#comment-15631199
]
Stefania commented on CASSANDRA-12791:
--
A dtest on trunk
[failed|http://cassci.datastax.com/view/Dev/view/stef1927/job/stef1927-12791-dtest/lastCompletedBuild/testReport/materialized_views_test/TestMaterializedViews/drop_column_test_2/]
because the node did not start, it didn't even produce any logs. I think it's
an unrelated test environment problem, but nonetheless I've relaunched the
dtests on trunk after squashing and rebasing.
I am assuming the 3.10 vote is going to fail, so I've put the patch under 3.10
in CHANGES.txt.
I will commit if the dtests on trunk are fine.
> 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 & 0xL) | (((partial
> & 0xL) << 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
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12791) MessageIn logic to determine if the message is cross-node is wrong
[
https://issues.apache.org/jira/browse/CASSANDRA-12791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15628229#comment-15628229
]
Sylvain Lebresne commented on CASSANDRA-12791:
--
I'm +1 with this last commit if CI is ok.
> 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 & 0xL) | (((partial
> & 0xL) << 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
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12791) MessageIn logic to determine if the message is cross-node is wrong
[
https://issues.apache.org/jira/browse/CASSANDRA-12791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15624015#comment-15624015
]
Stefania commented on CASSANDRA-12791:
--
I was trying to preserve the behavior of CASSANDRA-9793. However, it is true
that knowing if cross-node-timeout is enabled can be easily derived from yaml,
and I hadn't noticed that CASSANDRA-10580 added the latency to the same log
message. So I agree that it is better to have the number of dropped messages
and latency match.
I've amended the log message in [this
commit|https://github.com/stef1927/cassandra/commit/39168a3eb8e43815e4001521d2793d59c227f9ee].
CI still pending.
> 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 & 0xL) | (((partial
> & 0xL) << 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
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12791) MessageIn logic to determine if the message is cross-node is wrong
[
https://issues.apache.org/jira/browse/CASSANDRA-12791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15621787#comment-15621787
]
Sylvain Lebresne commented on CASSANDRA-12791:
--
bq. The intent is to help operators work out if messages are dropped because of
clock skew.
bq. we need to also check DD.hasCrossNodeTimeout(), a message originating cross
node is not sufficient.
I don't I agree tbh. Adding the {{DD.hasCrossNodeTimeout()}} check is losing
information and creating a somewhat confusing metric, but I disagree it's
really adding value. To quote Brandon on the original ticket, knowing if
messages are dropped of clock skew "is easily derived from the yaml". Namely,
if you do see a lot of cross-node dropped message but no local/internal ones,
then it's a fair sign this may be due to clock skew and you can then simply
check if {{DD.hasCrossNodeTimeout()}} is set or not to confirm.
So adding the {{DD.hasCrossNodeTimeout()}} check does not really add any
information that you can't easily infer otherwise, but adding it does mean that
when the option is {{false}} (the default as it happens), then the cross-node
metric will never-ever get incremented. And I can't shake the feeling that it's
going to be confusing for most users.I mean, they see we have 2 different
metrics, but only seeing lhe "local" one ever get incremented might make them
think only locally delivered message are dropped for some weird reason.
Anyway, I don't care tremendously about it (I was mostly bugged by the broken
logic in {{MessageIn}} after all) but I do think it's strictly better *without*
the check to {{DD.hasCrossNodeTimeout()}} in {{MS.incrementDroppedMessage()}}.
I'm good with the rest of the changes though.
> 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 & 0xL) | (((partial
> & 0xL) << 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
[jira] [Commented] (CASSANDRA-12791) MessageIn logic to determine if the message is cross-node is wrong
[
https://issues.apache.org/jira/browse/CASSANDRA-12791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15613928#comment-15613928
]
Stefania commented on CASSANDRA-12791:
--
Ci is clean, we had a jackpot in fact, 4/4 jobs with no failures. Just waiting
for Sylvain to take a final look at the code review changes before committing.
> 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 & 0xL) | (((partial
> & 0xL) << 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
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12791) MessageIn logic to determine if the message is cross-node is wrong
[
https://issues.apache.org/jira/browse/CASSANDRA-12791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15610550#comment-15610550
]
Stefania commented on CASSANDRA-12791:
--
Because [~slebresne] is out, I've rebased and applied the code review changes
to these branches:
||3.X||trunk||
|[patch|https://github.com/stef1927/cassandra/tree/12791-3.X]|[patch|https://github.com/stef1927/cassandra/tree/12791]|
|[testall|http://cassci.datastax.com/view/Dev/view/stef1927/job/stef1927-12791-3.X-testall/]|[testall|http://cassci.datastax.com/view/Dev/view/stef1927/job/stef1927-12791-testall/]|
|[dtest|http://cassci.datastax.com/view/Dev/view/stef1927/job/stef1927-12791-3.X-dtest/]|[dtest|http://cassci.datastax.com/view/Dev/view/stef1927/job/stef1927-12791-dtest/]|
CI is still pending.
> 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 & 0xL) | (((partial
> & 0xL) << 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
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12791) MessageIn logic to determine if the message is cross-node is wrong
[
https://issues.apache.org/jira/browse/CASSANDRA-12791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15582483#comment-15582483
]
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 & 0xL) | (((partial
> & 0xL) << 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
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-12791) MessageIn logic to determine if the message is cross-node is wrong
[
https://issues.apache.org/jira/browse/CASSANDRA-12791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15581110#comment-15581110
]
Stefania commented on CASSANDRA-12791:
--
The patch is fine, I'm not opposed to removing {{ConstructionTime}} given that
the meaning of {{isCrossNode}} now becomes a property of the message rather
than the timestamp. Also, I think 3.X is reasonable for this patch because
CASSANDRA-10580 was added in 3.2, see below.
{quote}
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.
{quote}
The confusion is due to the fact that {{ConstructionTime.isCrossNode}} was
intended to be a property of the construction timestamp, as it indicates
whether the timestamp originated at the sender or at the receiver, the intent
was not to indicate that the message itself is cross node, although comparing
the timestamps to determine this was incorrect. CASSANDRA-10580 added the
metrics in messaging service later on, and the comments in the code indicate
that the metrics refer to the actual messages being local vs. cross node, but
using {{isCrossNodeTimeout}} in messaging service was not correct.
{quote}
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.
{quote}
{{timestamp != crossNodeTimestamp}} orginates from this
[comment|https://issues.apache.org/jira/browse/CASSANDRA-9793?focusedCommentId=14635441=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14635441]
from CASSANDRA-9793. It is not only used for the metrics in MessagingService
but also for distinguishing messages dropped due to a cross node timeout vs. a
local timeout in the logs. The intent is to help operators work out if messages
are dropped because of clock skew. So, at this line
[here|https://github.com/pcmanus/cassandra/commit/0f3d7f6318be11b095bbe21d0c848da6409d1a93#diff-af09288f448c37a525e831ee90ea49f9R1204],
we need to also check {{DD.hasCrossNodeTimeout()}}, a message originating
cross node is not sufficient.
{{isCrossNodeTimeout}} is probably a misleading name now, and should become
simply {{isCrossNode}}.
I would like to give a heads up to [~brandon.williams] to make sure he agrees
that it's OK, from an operator point of view, to classify a dropped message in
the logs as dropped due to cross node timeout if:
* the message originates from a different node
* {{DD.hasCrossNodeTimeout()}} is true
given that it is extremely rare for a message to be received in the same
millisecond and have the machines perfectly synchronized and, if the machines
are not synchronized, then we can still have an identical timestamp due to
chance, so the best thing we can do is look at the yaml property.
As for the rest of the patch:
* There a typo at the end of this
[line|https://github.com/pcmanus/cassandra/commit/0f3d7f6318be11b095bbe21d0c848da6409d1a93#diff-2578da7d6bbdd276157604856543cbecR43],
the {{:}} should be {{;}}, this is the reason for the failures in
{{MonitoringTaskTest}} and all of the dtests.
* Another typo
[here|https://github.com/pcmanus/cassandra/commit/0f3d7f6318be11b095bbe21d0c848da6409d1a93#diff-70a9824fd63a5f3970840e376918da3eR137]
in the comments: {{lower}} -> {{higher}}.
> 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 & 0xL) | (((partial
> & 0xL) << 2) >> 2);
> if (timestamp > crossNodeTimestamp)
> {
>
