Re: [PR] JAVA-3057 Allow decoding a UDT that has more fields than expected [cassandra-java-driver]
akhaku commented on PR #1635: URL: https://github.com/apache/cassandra-java-driver/pull/1635#issuecomment-2131730576 Awesome, thank you for doing that work for me Lukasz! I added it in a new integration test since I couldn't find another existing one that fit. Also rebased. The case we ran into this issue had to do with the object mapper since it holds on to schema somewhere, and uses that to generate the cql queries for example. But this test is great too! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
Re: [PR] JAVA-3057 Allow decoding a UDT that has more fields than expected [cassandra-java-driver]
lukasz-antoniak commented on PR #1635:
URL:
https://github.com/apache/cassandra-java-driver/pull/1635#issuecomment-2122314815
I've had hard time trying to replicate this issue. Attempts to ignore schema
updates on control connection did not replicate the problem. In fact,
`DefaultCodecRegistry` will always attempt to create new codec if it does not
find corresponding one in the cache
([source](https://github.com/apache/cassandra-java-driver/blob/4.x/core/src/main/java/com/datastax/oss/driver/internal/core/type/codec/registry/DefaultCodecRegistry.java#L97)).
Cache key takes into account UDT name, but also names and types of fields.
This triggered my attention to the way application attempts to read UDT from
row. Leveraging `row.getUdtValue(...)` will again register new codec
([source](https://github.com/apache/cassandra-java-driver/blob/4.x/core/src/main/java/com/datastax/oss/driver/api/core/data/GettableByIndex.java#L128)).
However, trying to read the data with given `UdtCodec` will indeed expose the
issue. Since there is a public API method to read the column using certain
codec, I think that this issue is valid.
Below is the integration test that exposes the problem. Shall we add it to
the PR?
```java
/**
* @jira_ticket JAVA-3057
*/
@Test
public void should_decoding_udt_be_backward_compatible() {
CqlSession session = sessionRule.session();
session.execute("CREATE TYPE test_type_1 (a text, b int)");
session.execute("CREATE TABLE test_table_1 (e int primary key, f
frozen)");
// insert a row using version 1 of the UDT schema
session.execute("INSERT INTO test_table_1(e, f) VALUES(1, {a: 'a', b:
1})");
UserDefinedType udt = session
.getMetadata()
.getKeyspace(sessionRule.keyspace())
.flatMap(ks -> ks.getUserDefinedType("test_type_1"))
.orElseThrow(IllegalStateException::new);
TypeCodec oldCodec =
session.getContext().getCodecRegistry().codecFor(udt);
// update UDT schema
session.execute("ALTER TYPE test_type_1 add i text");
// insert a row using version 2 of the UDT schema
session.execute("INSERT INTO test_table_1(e, f) VALUES(2, {a: 'b', b: 2,
i: 'b'})");
Row row = session.execute("SELECT f FROM test_table_1 WHERE e = ?",
2).one();
// Try to read new row with old codec. Using row.getUdtValue() would not
cause any issues,
// because new codec will be automatically registered (using all 3
attributes).
// If application leverages generic row.get(String, Codec) method, data
reading with old codec should
// be backward-compatible.
UdtValue value = (UdtValue) row.get("f", oldCodec);
assertThat(value.getString("a")).isEqualTo("b");
assertThat(value.getInt("b")).isEqualTo(2);
assertThatThrownBy(() -> value.getString("i")).hasMessage("i is not a
field in this UDT");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
Re: [PR] JAVA-3057 Allow decoding a UDT that has more fields than expected [cassandra-java-driver]
akhaku commented on code in PR #1635:
URL:
https://github.com/apache/cassandra-java-driver/pull/1635#discussion_r1525623390
##
core/src/main/java/com/datastax/oss/driver/internal/core/type/codec/UdtCodec.java:
##
@@ -105,10 +105,7 @@ public UdtValue decode(@Nullable ByteBuffer bytes,
@NonNull ProtocolVersion prot
int i = 0;
while (input.hasRemaining()) {
if (i == cqlType.getFieldTypes().size()) {
- throw new IllegalArgumentException(
- String.format(
- "Too many fields in encoded UDT value, expected %d",
- cqlType.getFieldTypes().size()));
+ break;
Review Comment:
It's been a while but my memory is a little fuzzy as to the exact scenario
with which we ran into this, but it had to do with the object mapper caching
schemas somewhere too. In any case, the client metadata refresh hint sounds
reasonable for when we tackle what you said earlier about EVENT delivery
failing, since that would be the logical place for the hint to propagate and
affect. For now I'll add a debug log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
Re: [PR] JAVA-3057 Allow decoding a UDT that has more fields than expected [cassandra-java-driver]
aratno commented on code in PR #1635:
URL:
https://github.com/apache/cassandra-java-driver/pull/1635#discussion_r1524601669
##
core/src/main/java/com/datastax/oss/driver/internal/core/type/codec/UdtCodec.java:
##
@@ -105,10 +105,7 @@ public UdtValue decode(@Nullable ByteBuffer bytes,
@NonNull ProtocolVersion prot
int i = 0;
while (input.hasRemaining()) {
if (i == cqlType.getFieldTypes().size()) {
- throw new IllegalArgumentException(
- String.format(
- "Too many fields in encoded UDT value, expected %d",
- cqlType.getFieldTypes().size()));
+ break;
Review Comment:
Thought about this some more. The driver seeing extra fields in a UDT is
expected for a short period after a schema change to add fields, since the
control connection may find out later than the coordinator or replicas, and
EVENTs are sent over the control connection async. It may be worth a warning if
this state persists for too long, since that's an indication that a schema
change EVENT wasn't delivered, but delivery failures are expected to be
tolerated as well.
I think it makes sense to drop this to a DEBUG or TRACE log, rather than
using a no-spam logger.
In the more general case of EVENT delivery failing and client metadata going
out of sync, there's more we could do to retry EVENTs or support bounded
metadata lag, but all of that is out of scope for this ticket.
The only other change I'm considering is whether we should add a hint to
refresh client metadata in this situation where the server has indicated that
it has a new schema the client isn't yet aware of. Metadata refreshes should be
debounced so we don't attempt a refresh on every response decode. Refreshing
client metadata would only help in the case when the control instance does know
about the schema but EVENT delivery failed, not in cases where the control
instance has not yet found out about the new schema but a particular
coordinator has, which is more likely the case for larger clusters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
Re: [PR] JAVA-3057 Allow decoding a UDT that has more fields than expected [cassandra-java-driver]
akhaku commented on code in PR #1635:
URL:
https://github.com/apache/cassandra-java-driver/pull/1635#discussion_r1524006033
##
core/src/main/java/com/datastax/oss/driver/internal/core/type/codec/UdtCodec.java:
##
@@ -105,10 +105,7 @@ public UdtValue decode(@Nullable ByteBuffer bytes,
@NonNull ProtocolVersion prot
int i = 0;
while (input.hasRemaining()) {
if (i == cqlType.getFieldTypes().size()) {
- throw new IllegalArgumentException(
- String.format(
- "Too many fields in encoded UDT value, expected %d",
- cqlType.getFieldTypes().size()));
+ break;
Review Comment:
Yes, that's the scenario we ran into, and I can't think of any other
scenarios.
Fair, a no-spam warn is a good idea but it doesn't look like the client
currently has a no-spam logger. I could copy the one from the server but maybe
not the best thing to do in this PR... and a vanilla log warn would be bad here
since it would get logged every time we decode a UDT field matching that
scenario. Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
Re: [PR] JAVA-3057 Allow decoding a UDT that has more fields than expected [cassandra-java-driver]
aratno commented on code in PR #1635:
URL:
https://github.com/apache/cassandra-java-driver/pull/1635#discussion_r1523809556
##
core/src/main/java/com/datastax/oss/driver/internal/core/type/codec/UdtCodec.java:
##
@@ -105,10 +105,7 @@ public UdtValue decode(@Nullable ByteBuffer bytes,
@NonNull ProtocolVersion prot
int i = 0;
while (input.hasRemaining()) {
if (i == cqlType.getFieldTypes().size()) {
- throw new IllegalArgumentException(
- String.format(
- "Too many fields in encoded UDT value, expected %d",
- cqlType.getFieldTypes().size()));
+ break;
Review Comment:
I can understand not failing in this case, but logging a (no-spam) warning
seems appropriate. Based on my understanding, this should only happen if a
schema change happens on the server to add fields to a UDT, and the client gets
an updated UDT in a response before finding out about the new schema.
Are there any other situations when this might occur?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
