[jira] [Comment Edited] (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&focusedCommentId=16260641#comment-16260641
]
Alex Petrov edited comment on CASSANDRA-13992 at 11/21/17 12:18 PM:
Thank you for the review!
Committed to trunk with
[7eb915097dc3e34e1bb4ef96e6bd8eb67d574622|https://github.com/apache/cassandra/commit/7eb915097dc3e34e1bb4ef96e6bd8eb67d574622]
with added unit test and {{hasConditions}} pulled up to statement.
was (Author: ifesdjeen):
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: 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] [Comment Edited] (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&focusedCommentId=16255351#comment-16255351
]
Robert Stupp edited comment on CASSANDRA-13992 at 11/16/17 1:57 PM:
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()}}.
was (Author: snazy):
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
[jira] [Comment Edited] (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&focusedCommentId=16253220#comment-16253220
]
Kurt Greaves edited comment on CASSANDRA-13992 at 11/15/17 10:11 AM:
-
bq. 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.
was (Author: kurtg):
> 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] [Comment Edited] (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&focusedCommentId=16251096#comment-16251096
]
Alex Petrov edited comment on CASSANDRA-13992 at 11/14/17 8:44 AM:
---
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.
[~KurtG] to give a bit of context, {{METADATA_CHANGED}} flag is instructing the
driver to cache a newly received version of metadata (alongside with a new
metadata ID). While {{SKIP_METADATA}} flag is hinting the driver to use already
cached metadata from previous responses. If I understood it correctly, what
[~omichallat] proposed here was to avoid setting {{METADATA_CHANGED}} flag, so
driver wouldn't cache the metadata, _but_ still send a metadata all the time
(since it's potentially changing on each request).
bq. 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?
The patch can be found:
|[here|https://github.com/apache/cassandra/compare/trunk...ifesdjeen:CASSANDRA-13992]|
was (Author: ifesdjeen):
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.
[~KurtG] to give a bit of context, {{METADATA_CHANGED}} flag is instructing the
driver to cache a newly received version of metadata (alongside with a new
metadata ID). While {{SKIP_METADATA}} flag is hinting the driver to use already
cached metadata from previous responses. If I understood it correctly, what
[~omichallat] proposed here was to avoid setting {{METADATA_CHANGED}} flag, so
driver wouldn't cache the metadata, _but_ still send a metadata all the time
(since it's potentially changing on each request).
bq. 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, ev
[jira] [Comment Edited] (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&focusedCommentId=16251096#comment-16251096
]
Alex Petrov edited comment on CASSANDRA-13992 at 11/14/17 8:43 AM:
---
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.
[~KurtG] to give a bit of context, {{METADATA_CHANGED}} flag is instructing the
driver to cache a newly received version of metadata (alongside with a new
metadata ID). While {{SKIP_METADATA}} flag is hinting the driver to use already
cached metadata from previous responses. If I understood it correctly, what
[~omichallat] proposed here was to avoid setting {{METADATA_CHANGED}} flag, so
driver wouldn't cache the metadata, _but_ still send a metadata all the time
(since it's potentially changing on each request).
bq. 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?
was (Author: ifesdjeen):
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.
bq. 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] [Comment Edited] (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&focusedCommentId=16251096#comment-16251096
]
Alex Petrov edited comment on CASSANDRA-13992 at 11/14/17 8:39 AM:
---
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.
bq. 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?
was (Author: ifesdjeen):
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] [Comment Edited] (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&focusedCommentId=16250071#comment-16250071
]
Alex Petrov edited comment on CASSANDRA-13992 at 11/13/17 7:43 PM:
---
[~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.
I haven't updated spec though. I will, if/when we agree on the behaviour.
was (Author: ifesdjeen):
[~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] [Comment Edited] (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&focusedCommentId=16249838#comment-16249838
]
Olivier Michallat edited comment on CASSANDRA-13992 at 11/13/17 5:18 PM:
-
[~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 explain that in
the protocol spec:
{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 updates, isn't there any way we can do that?
was (Author: omichallat):
[~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] [Comment Edited] (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&focusedCommentId=16245469#comment-16245469
]
Alex Petrov edited comment on CASSANDRA-13992 at 11/9/17 10:29 AM:
---
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. 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). 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.
was (Author: ifesdjeen):
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
