[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16284041#comment-16284041
]
Olivier Michallat commented on CASSANDRA-13992:
---
Yes, with the algorithm I described in [this
comment|https://issues.apache.org/jira/browse/CASSANDRA-13992?focusedCommentId=16253022=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16253022],
LWTs are handled exactly the same way as regular statements, so I don't think
we need to amend the spec.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Alex Petrov
>Priority: Minor
> Fix For: 4.0
>
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16283448#comment-16283448
]
Alex Petrov commented on CASSANDRA-13992:
-
We didn't do any client-side changes. As far as I can understand, metadata
changes are server-driven.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Alex Petrov
>Priority: Minor
> Fix For: 4.0
>
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16282724#comment-16282724
]
Kurt Greaves commented on CASSANDRA-13992:
--
Should we have updated the spec to indicate that those flags will not work with
LWT? Seems like it could be quite surprising for new driver developers.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Alex Petrov
>Priority: Minor
> Fix For: 4.0
>
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16260641#comment-16260641
]
Alex Petrov commented on CASSANDRA-13992:
-
Thank you for the review!
Committed to trunk with
[7eb915097dc3e34e1bb4ef96e6bd8eb67d574622|https://github.com/apache/cassandra/commit/7eb915097dc3e34e1bb4ef96e6bd8eb67d574622]
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
> Fix For: 4.0
>
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16255351#comment-16255351
]
Robert Stupp commented on CASSANDRA-13992:
--
As elaborated above, LWTs definitely need special handling as their result set
is (or can be) different for each invocation. Remembering (and evicting)
metadata for that result set (which is in the "ok" case just one column
{{[applied]}} - i.e. not much) is probably not beneficiary. Doing that might be
worth another look in a separate ticket at a later point. But I don't expect
much from that kind of optimization.
I'm not excited about adding a "special value" to indicate that the metadata is
empty (neither an empty {{byte[]}} nor the MD5 over an empty {{byte[]}}). Just
omitting the metadata flags introduced by CASSANDRA-10786 is sufficient.
Olivier correctly pointed out that (unnecessary) additional processing should
be avoided - and I second that. Looking at the {{METADATA_CHANGED}} flag is
very cheap - comparing values is more expensive (checking a bit in a CPU
register or L1 cache line vs. many dloads). Keeping the performance aspect
aside, it also looks cleaner.
Since we do not need those metadata-flags for LWTs, we can just omit those and
it "magically" works - and that's pretty much what [~ifesdjeen]'s patch does.
I've written a [unit
test|https://github.com/snazy/cassandra/commit/fcb221af2dcc74c57e3017b73937365e2226b7d3#diff-d04861816aec1bdaa47b3d6819df1a46R277]
that verifies the expected behavior on the protocol level.
+1 on [~ifesdjeen]'s patch. I'd like to see the new unit test being added.
Only change that would be good to have is to move the {{boolean
hasConditions()}} function up to {{CQLStatement}} and implement it there as
{{public default boolean hasConditions() { return false; } }}. By that you can
remove the {{instanceof}} and type casts in the change in {{ExecuteMessage}}
and probably also save the {{isLWT}} variable as it would just be a call to
{{statement.hasConditions()}}.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16254639#comment-16254639
]
Kurt Greaves commented on CASSANDRA-13992:
--
bq. But then I would have to compare the ids every time. That costs me a lookup
in my client-side prepared statement cache (and there also happens to be an
additional volatile read in our current implementation). In contrast, checking
METADATA_CHANGED is free since it is already in the response.
OK I get it.
bq. I thought we have discussed it and I've tried this idea out and it doesn't
solve the problem with actually updating the metadata when performing ALTER.
Just trying to understand things here but from what I can see the patches both
provide the same behaviour. Notably, METADATA_CHANGED will never be set for LWT
but the metadata will still always be returned to the client. Anyway, you can
do it that way if you want seeing as it helps on the java driver side.
But now that I think about it more and after some testing I don't see how
either case works completely for {{ALTER}}. When you say ALTER what are you
referring to? AFAICT all you can do is add + drop columns that aren't used in
the prepared statement, and atm both patches behave in the exact same way. If
you alter any column used in the statement it invalidates the statement and you
get an error. What aspects of ALTER are you expecting to work?
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16253591#comment-16253591
]
Olivier Michallat commented on CASSANDRA-13992:
---
[~KurtG]
bq. You wouldn't have to worry about METADATA_CHANGED being set because a LWT
will always have the same ID as the initial preparation
But then I would have to compare the ids every time. That costs me a lookup in
my client-side prepared statement cache (and there also happens to be an
additional volatile read in our current implementation). In contrast, checking
METADATA_CHANGED is free since it is already in the response.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16253236#comment-16253236
]
Alex Petrov commented on CASSANDRA-13992:
-
I thought we have discussed it and I've tried this idea out and it doesn't
solve the problem with actually updating the metadata when performing {{ALTER}}.
This is a problem with many pitfalls, I've done a lot of testing both back when
wrote an initial patch and now when we were collaborating on the follow-up and
personally believe the proposed patch solves a problem in the best possible
way. I'm happy to hear all the alternatives out as long as they're tested out
and confirmed to work for all cases and keep consistency with previous versions.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16253220#comment-16253220
]
Kurt Greaves commented on CASSANDRA-13992:
--
> then I have to change the last test to if (METADATA_CHANGED ||
> new_metadata_id == special_value)
In Alex's last version, yes, but if the metadata ID from the prepare and the
exec are the same (because they are generated with the same value) this is not
the case. You wouldn't have to worry about METADATA_CHANGED being set because a
LWT will always have the same ID as the initial preparation.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16253188#comment-16253188
]
Alex Petrov commented on CASSANDRA-13992:
-
[~snazy] could you take a look at the patch as you're most familiar with the
previous version and we've discussed the behaviour for LWTs multiple times?
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16253022#comment-16253022
]
Olivier Michallat commented on CASSANDRA-13992:
---
[~ifesdjeen] yes, your last patch is fine for me from a client's perspective (I
can't really comment on the implementation since I'm not that familiar with the
Cassandra codebase).
[~KurtG]:
bq. you can't skip metadata for LWT
Nor should you be able to. You can't safely skip metadata since these
statements may return different columns depending on the state of the database.
You _have_ to return the metadata every time.
bq. metadata will never be changed for LWT
We don't need it since every response includes the correct metadata.
bq. AFAICT Alex Petrov's patch only really works because the driver is already
set up for it to work.
Actually no, it allows the driver to treat LWT as any other statement. Here's
roughly what we do:
{code}
// When decoding a PREPARED response
if ! NO_METADATA // Note that LWT always have NO_METADATA
store result metadata in prepared statement cache
end if
// When decoding a ROWS response:
if NO_METADATA
look up result metadata in prepared statement cache
else
decode result metadata from response
if (METADATA_CHANGED)
update result metadata in prepared statement cache
end if
end if
{code}
So unsetting {{METADATA_CHANGED}} for LWT allows me to properly skip the last
cache update. If we use a special value of {{new_metadata_id}} instead (like
the empty array in the initial patch), then I have to change the last test to
{{if (METADATA_CHANGED || new_metadata_id == special_value)}}.
HTH
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16252848#comment-16252848
]
Kurt Greaves commented on CASSANDRA-13992:
--
I don't see why special casing LWT is necessary. It couples LWT,
{{ModificationStatement}} and {{BatchStatement}} with {{ExecuteMessage}}, which
seems a bit messy. It also introduces a weird edge case into the protocol -
i.e,
1. you can't skip metadata for LWT
2. metadata will never be changed for LWT.
2 is fine as it's somewhat the goal of the ticket, but when we already have a
mechanism to meet this requirement it seems silly to wrap it in another. 1 just
adds complexity to the protocol, which imo, is complex enough. It's not even
clear in the protocol that SKIP_METADATA will be ignored if the metadata *did*
change (we should update this in the patch). I think this can be better solved
simply by providing a consistent MD5 for LWT's as my patch already does
[here|https://github.com/apache/cassandra/compare/trunk...kgreav:13992]. This
removes the need to introduce new imports, and special casing to
{{ExecuteMessage}} while achieving the same behaviour, and I think logically is
easier to explain.
AFAICT [~ifesdjeen]'s patch only really works because the driver is already set
up for it to work. The _java_ driver already checks that {{new_metadata_id !=
null}} which is logical, but in no way required. It seems to me we're
introducing a metadata_id that could potentially be null where a driver might
expect it to have a value. I prefer my patch, or better yet, we create an
{{EMPTY_DIGEST}} that's an actual MD5 to use so we don't have to rely on
{{compute}} every time we want to use an {{EMPTY_DIGEST}}
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16251096#comment-16251096
]
Alex Petrov commented on CASSANDRA-13992:
-
bq. METADATA_CHANGED tells the client if it needs to update its local copy of
the metadata.
You're right, great point. Sure, I've changed the patch to _always_ send
metadata (by avoiding setting {{SKIP_METADATA}} for LWTs) and _never_ send
metadata id when statement is LWT (by avoiding setting {{METADATA_CHANGED}} for
LWTs)). Technically, {{EMPTY}} part isn't even necessary in that case, but we
don't have to calculate it, so why not.
> I'm with Olivier that that's a hacky addition to the driver
There's no addition to the driver (also, was no addition in the previous
version of the patch). The only difference is that we can spare the driver a
couple of cycles. Behaviour was right in both cases.
I've pulled in the last version of the driver, added comments and prettified it
a bit. If we all agree that this behaviour is correct, can anyone take a short
look at it?
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16250786#comment-16250786
]
Kurt Greaves commented on CASSANDRA-13992:
--
My understanding is that, at the moment, {{METADATA_CHANGED}} will _always_ be
set for a conditional update, regardless of whether it's necessary or not.
Necessary being defined as the schema has actually changed and the prepared
statements need to be updated client side to reflect those schema changes.
[~omichallat] is this true? what exactly is "metadata" referring to on the
driver side, and why is the answer "always no" for conditional updates? If
there is a change to one of the columns in the update is that going to cause
problems if we don't tell the driver that it has changed?
I'm with Olivier that that's a hacky addition to the driver, but if it's not
even necessary as per above then simply only passing an empty digest will be
sufficient.
I've updated my
[branch|https://github.com/apache/cassandra/compare/trunk...kgreav:13992] to
reflect this. Note I've changed to using {{MD5Digest#compute}} to calculate an
"empty" digest. Although it's thread local it will always be the same digest,
and this will also solve the initial preparation problem, as it also uses the
{{EMPTY}} resultset + metadata.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16250154#comment-16250154
]
Olivier Michallat commented on CASSANDRA-13992:
---
{{METADATA_CHANGED}} tells the client if it needs to update its local copy of
the metadata. For conditional updates, the answer is always no (since the
client should never store that information in the first place); that is why I
think it's more intuitive to set the flag to false.
To put it another way: if the flag is forced to true, I have to add a condition
in the client code ({{newMetadataId.bytes.length > 0}}). My worry is that a
client implementation could forget to check that the id is empty, and end up
with a sub-optimal behavior (that updates the local metadata unnecessarily each
time).
If the flag is absent, conditional updates can be handled like any other
statement.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16250071#comment-16250071
]
Alex Petrov commented on CASSANDRA-13992:
-
[~omichallat] not sure, since {{METADATA_CHANGED}} is just a flag: e.g. if it's
set it's {{true}}, otherwise it's {{false}}. Moreover, I think that the default
behaviour for LWTs has to be that we _always_ update metadata: there's no way
for server to know what was the last metadata on the client (since it depends
on the result), the server can't distinguish between the metadata hash
inequality caused by {{ALTER}} vs caused by success/non-success LWT result.
Unless I'm missing something, my patch achieves exactly that (also, without any
driver changes): it forces the server to _always_ send the metadata. This,
combined with the metadata consisting of zeroes can instruct the client that
caching metadata is possible, but won't bring anything: new result metadata
will just be re-delivered on every call, since it's potentially going to be
changing on every request.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16249838#comment-16249838
]
Olivier Michallat commented on CASSANDRA-13992:
---
[~ifesdjeen] that would work, the driver can treat an empty {{new_metadata_id}}
as "don't update my local copy". Namely, changing [this
line|https://github.com/datastax/java-driver/blob/6eeb8b2193ab5b50b73b0d9a533e775265f11007/driver-core/src/main/java/com/datastax/driver/core/ArrayBackedResultSet.java#L83]
to:
{code}
if (newMetadataId != null && newMetadataId.bytes.length > 0) {
{code}
However that feels kind of hacky. Consider how we would have to update the
protocol spec to explain this:
{quote}
- is \[short bytes] representing the new, changed
resultset
metadata. The new metadata ID must also be used in subsequent
executions of
the corresponding prepared statement, if any, *except if it is
empty*.
{quote}
It would make so much more sense to force {{METADATA_CHANGED}} to *false* for
conditional statements, isn't there any way we can do that?
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16249215#comment-16249215
]
Alex Petrov commented on CASSANDRA-13992:
-
I've composed a version of the patch, to demonstrate my thinking
[here|https://github.com/apache/cassandra/compare/trunk...ifesdjeen:CASSANDRA-13992].
It seems that we can solve this problem without patching the driver. In fact,
it might be even better if inner doings of metadata hash are transparent for
the driver.
In short, we can always force {{METADATA_CHANGED}} for conditional statements
and avoid computing their metadata to make sure it's empty. It's a rough
equivalent of making metadata hash random, just simpler to reason about. What
do you think about it [~KurtG] [~omichallat]
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16249071#comment-16249071
]
Olivier Michallat commented on CASSANDRA-13992:
---
bq. Before I waste a lot of time figuring out how to test this in the driver,
can you point me at how you did it?
The driver-side changes are not merged yet, but you can find them in [this pull
request|https://github.com/datastax/java-driver/pull/794]. The test that covers
this specific scenario is
[PreparedStatementInvalidationTest#should_never_update_statement_id_for_conditional_updates_in_modern_protocol|https://github.com/datastax/java-driver/blob/46825e446ae9d5f57baeb4f5f2c1f5fc4b99d972/driver-core/src/test/java/com/datastax/driver/core/PreparedStatementInvalidationTest.java#L182].
To run the test you'll need to install CCM (see some instructions
[here|https://github.com/datastax/java-driver/blob/3.3.x/CONTRIBUTING.md#running-the-tests]).
Then because you're not testing a released Cassandra version, you'll need to
point the test harness to your local working copy with those system properties:
{{-Dcassandra.version=4.0.0 -Dcassandra.directory=/path/to/cassandra}}
If you want to do some debugging, the result metadata is handled in
{{ArrayBackedResultSet.java}}. Search for "CASSANDRA-13992", there is a comment
that explains what should be changed if this ticket is fixed.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16246907#comment-16246907
]
Kurt Greaves commented on CASSANDRA-13992:
--
So it seems that prepare does return an empty digest, however
{{ResultSet.ResultMetadata.EMPTY}} is created with {{compute}} and is thus a
thread local digest, which will get a different hash. From what I can see EMPTY
is only ever used in one place worth mentioning, and that's in
{{org.apache.cassandra.cql3.ResultSet.ResultMetadata#fromPrepared}} (it's also
used in {{org.apache.cassandra.transport.Message.Codec#decode}} but only for
protocol versions prior to V1), and will be used for anything that's not a
{{SELECT}} statement.
Now, what I'm wondering is there any significant reason the ID for an "EMPTY"
metadata ID is created as a thread local digest?
If we can change empty to instead use {{MD5Digest.wrap()}} we solve the
prepared problem.
[~omichallat] Before I waste a lot of time figuring out how to test this in the
driver, can you point me at how you did it?
bq. Since in this case it seems what will happen is when table is ALTER'ed,
metadata won't update (I haven't checked it though)
Yes we'll probably need to do something about this. I'll write up some tests
first.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16245469#comment-16245469
]
Alex Petrov commented on CASSANDRA-13992:
-
If we take this patch, I'd definitely constantize the "empty" hash, as very
least.
Other than that - I think we should add more tests with LWTs (preferably
dtests) and check what happens when we actually alter the table. In the initial
patch we've tried always forcing metadata transfer for LWTs. If the patch has
the same effect (metadata is transferred every time). I'm not insisting on any
particular solution here though.
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16244458#comment-16244458
]
Olivier Michallat commented on CASSANDRA-13992:
---
bq. If C* thinks the metadata changed it will set the METADATA_CHANGED flag and
the driver should need to update it's metadata. TBH this isn't super clear from
the spec but appears to be what the code achieves here.
That makes sense, and indeed explains why the server ignores SKIP_METADATA.
I've tested your patch against a driver snapshot. The bound statement
executions now always return the same (empty) digest, but the problem is that
the initial preparation still returns a non-empty digest. So the driver
executes with that initial digest and gets METADATA_CHANGED with the empty
digest every time.
The prepare should also return an empty digest, which I think should be done
around
[here|https://github.com/kgreav/cassandra/blob/fa259fd79ea3e0a0fac8583a54c9c76464a653be/src/java/org/apache/cassandra/cql3/QueryProcessor.java#L444].
cc [~ifesdjeen]
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Assignee: Kurt Greaves
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13992) Don't send new_metadata_id for conditional updates
[
https://issues.apache.org/jira/browse/CASSANDRA-13992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16241847#comment-16241847
]
Kurt Greaves commented on CASSANDRA-13992:
--
bq. The next EXECUTE is sent with SKIP_METADATA = true, but the server appears
to ignore that
I believe this is because METADATA_CHANGED will take precedence. If C* thinks
the metadata changed it will set the METADATA_CHANGED flag and the driver
should need to update it's metadata. TBH this isn't super clear from the spec
but appears to be what the code achieves
[here|https://github.com/apache/cassandra/blob/922dbdb658b1693973926026b213153d05b4077c/src/java/org/apache/cassandra/transport/messages/ExecuteMessage.java#L174].
I may have no idea what I'm talking about but I think the simplest solution to
bq. never send a new_metadata_id in for a conditional update.
would be to simply always use the same digest for any LWT.
I think the following patch achieves this without breaking anything but I
haven't confirmed if it actually fixes the driver issue yet. If someone with
more understanding of the protocol and what not could have a glance and let me
know if this makes sense or point me in the right direction.
[trunk|https://github.com/apache/cassandra/compare/trunk...kgreav:13992-trunk]
> Don't send new_metadata_id for conditional updates
> --
>
> Key: CASSANDRA-13992
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13992
> Project: Cassandra
> Issue Type: Bug
>Reporter: Olivier Michallat
>Priority: Minor
>
> This is a follow-up to CASSANDRA-10786.
> Given the table
> {code}
> CREATE TABLE foo (k int PRIMARY KEY)
> {code}
> And the prepared statement
> {code}
> INSERT INTO foo (k) VALUES (?) IF NOT EXISTS
> {code}
> The result set metadata changes depending on the outcome of the update:
> * if the row didn't exist, there is only a single column \[applied] = true
> * if it did, the result contains \[applied] = false, plus the current value
> of column k.
> The way this was handled so far is that the PREPARED response contains no
> result set metadata, and therefore all EXECUTE messages have SKIP_METADATA =
> false, and the responses always include the full (and correct) metadata.
> CASSANDRA-10786 still sends the PREPARED response with no metadata, *but the
> response to EXECUTE now contains a {{new_metadata_id}}*. The driver thinks it
> is because of a schema change, and updates its local copy of the prepared
> statement's result metadata.
> The next EXECUTE is sent with SKIP_METADATA = true, but the server appears to
> ignore that, and still sends the metadata in the response. So each response
> includes the correct metadata, the driver uses it, and there is no visible
> issue for client code.
> The only drawback is that the driver updates its local copy of the metadata
> unnecessarily, every time. We can work around that by only updating if we had
> metadata before, at the cost of an extra volatile read. But I think the best
> thing to do would be to never send a {{new_metadata_id}} in for a conditional
> update.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
