KAFKA-10675: Add schema name to ConnectSchema.validateValue() error message #9541

Iskuskov · 2020-11-02T22:34:14Z

https://issues.apache.org/jira/browse/KAFKA-10675

The following error message

org.apache.kafka.connect.errors.DataException: Invalid Java object for schema type INT64: class java.lang.Long for field: "moderate_time"

can be confusing because java.lang.Long is acceptable type for schema INT64.

In fact, in this case org.apache.kafka.connect.data.Timestamp is used but this info is not logged.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

Iskuskov · 2021-02-20T23:44:52Z

@C0urante, @gharris1727, could you take a look at this please?

C0urante · 2021-03-01T15:34:43Z

connect/api/src/main/java/org/apache/kafka/connect/data/ConnectSchema.java

+    private static String buildSchemaInfo(Schema schema) {
+        StringBuilder schemaInfo = new StringBuilder("schema");
+        if (schema.name() != null) {
+            schemaInfo.append(" \"").append(schema.name()).append("\"");
+        }
+        schemaInfo.append(" with type ").append(schema.type());
+        return schemaInfo.toString();
+    }


Given that this is only used in one place, and the string that it creates is itself added to a formatted string, I think it'd be a bit more readable if we removed this method and inlined all the logic where it's being invoked right now.

Thanks for reviewing! Fixes were pushed

C0urante

LGTM, one comment about readability but you can take it or leave it.

I'm also wondering if we might consider dropping the " for field ..." part from the exception message if field is null (which occurs with the two-argument variant of ConnectSchema::validateValue), but that can be left as follow-up work if you'd prefer.

Iskuskov · 2021-03-05T08:44:38Z

@kkonstantine, could you take a look at this please?

Iskuskov · 2021-03-30T12:01:49Z

@rhauch, could you take a look at this please?

Iskuskov · 2021-04-15T17:15:41Z

@chia7712, @ijuma, could you take a look at this please?

rhauch

Thanks, @Iskuskov. LGTM.

We're technically still frozen on the 2.6 and 2.8 branches, but both have RCs that passed their PMC votes and should be unblocked soon. I'll hopefully be able to merge and backport soon.

rhauch · 2021-04-19T15:58:51Z

connect/api/src/main/java/org/apache/kafka/connect/data/ConnectSchema.java

@@ -266,7 +267,7 @@ public static void validateValue(String name, Schema schema, Object value) {
    private static List<Class> expectedClassesFor(Schema schema) {
        List<Class> expectedClasses = LOGICAL_TYPE_CLASSES.get(schema.name());
        if (expectedClasses == null)
-            expectedClasses = SCHEMA_TYPE_CLASSES.get(schema.type());
+            expectedClasses = SCHEMA_TYPE_CLASSES.getOrDefault(schema.type(), Collections.emptyList());


Strictly speaking, this shouldn't be necessary as SCHEMA_TYPE_CLASSES should have a Schema instance for all Schema.Type literals. And with SchemaBuilder a connector or converter cannot create a schema instance with a null Schema.Type.

However, it is possible to construct a ConnectSchema instance with a null Type reference (like what FakeSchema essentially does in the existing test), which of course without this change would result in this method returning a null list.

So +1 for this line change since it simplifies the error handling in the calling code.

Thank you! Highly appreciate your comment

kkonstantine · 2021-07-09T03:33:33Z

@rhauch seems that this fix was approved but we were blocked on release candidates of previous releases. To include to 3.0 (as well as previous releases) and avoid entering a new code freeze phase we'd need to merge asap. Please take another look if possible.

@Iskuskov in the meantime a merge conflict has been introduced. Would you mind rebasing on top of the latest changes on trunk?

…ateValue()

Iskuskov · 2021-07-09T09:30:42Z

@Iskuskov in the meantime a merge conflict has been introduced. Would you mind rebasing on top of the latest changes on trunk?

@kkonstantine thank you! Rebased

kkonstantine

We recently enabled warnings on the use of raw types. That's why the build failed after the rebase.
Adding a suggestion and merging to the PR to allow the tests to run.

connect/api/src/main/java/org/apache/kafka/connect/data/ConnectSchema.java

kkonstantine

Test failures are not relevant.
LGTM

…essage (#9541) The following error message `org.apache.kafka.connect.errors.DataException: Invalid Java object for schema type INT64: class java.lang.Long for field: "moderate_time"` can be confusing because java.lang.Long is acceptable type for schema INT64. In fact, in this case `org.apache.kafka.connect.data.Timestamp` is used but this info is not logged. Reviewers: Randall Hauch <[email protected]>, Chris Egerton <[email protected]>, Konstantine Karantasis <[email protected]>

…essage (apache#9541) The following error message `org.apache.kafka.connect.errors.DataException: Invalid Java object for schema type INT64: class java.lang.Long for field: "moderate_time"` can be confusing because java.lang.Long is acceptable type for schema INT64. In fact, in this case `org.apache.kafka.connect.data.Timestamp` is used but this info is not logged. Reviewers: Randall Hauch <[email protected]>, Chris Egerton <[email protected]>, Konstantine Karantasis <[email protected]>

kkonstantine added the connect label Nov 3, 2020

Iskuskov force-pushed the KAFKA-10675 branch 2 times, most recently from b6912b9 to 12ddffd Compare February 20, 2021 22:45

C0urante reviewed Mar 1, 2021

View reviewed changes

C0urante approved these changes Mar 1, 2021

View reviewed changes

Iskuskov force-pushed the KAFKA-10675 branch 2 times, most recently from 9cfcb66 to c8a361c Compare March 4, 2021 20:53

rhauch approved these changes Apr 19, 2021

View reviewed changes

Iskuskov added 3 commits July 9, 2021 12:12

KAFKA-10675 Add schema name to error message from ConnectSchema.valid…

468790f

…ateValue()

KAFKA-10675 refactoring

ea28af7

KAFKA-10675 fix after review

3c25fdf

Iskuskov force-pushed the KAFKA-10675 branch from c8a361c to 3c25fdf Compare July 9, 2021 09:28

kkonstantine reviewed Jul 9, 2021

View reviewed changes

connect/api/src/main/java/org/apache/kafka/connect/data/ConnectSchema.java Outdated Show resolved Hide resolved

Don't use raw type for Class

a5b765f

kkonstantine approved these changes Jul 10, 2021

View reviewed changes

kkonstantine merged commit efe6029 into apache:trunk Jul 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-10675: Add schema name to ConnectSchema.validateValue() error message #9541

KAFKA-10675: Add schema name to ConnectSchema.validateValue() error message #9541

Iskuskov commented Nov 2, 2020 •

edited

Loading

Iskuskov commented Feb 20, 2021

C0urante Mar 1, 2021

Iskuskov Mar 4, 2021

C0urante left a comment

Iskuskov commented Mar 5, 2021

Iskuskov commented Mar 30, 2021

Iskuskov commented Apr 15, 2021

rhauch left a comment

rhauch Apr 19, 2021

Iskuskov Apr 21, 2021

kkonstantine commented Jul 9, 2021

Iskuskov commented Jul 9, 2021

kkonstantine left a comment •

edited

Loading

kkonstantine left a comment

KAFKA-10675: Add schema name to ConnectSchema.validateValue() error message #9541

KAFKA-10675: Add schema name to ConnectSchema.validateValue() error message #9541

Conversation

Iskuskov commented Nov 2, 2020 • edited Loading

Committer Checklist (excluded from commit message)

Iskuskov commented Feb 20, 2021

C0urante Mar 1, 2021

Choose a reason for hiding this comment

Iskuskov Mar 4, 2021

Choose a reason for hiding this comment

C0urante left a comment

Choose a reason for hiding this comment

Iskuskov commented Mar 5, 2021

Iskuskov commented Mar 30, 2021

Iskuskov commented Apr 15, 2021

rhauch left a comment

Choose a reason for hiding this comment

rhauch Apr 19, 2021

Choose a reason for hiding this comment

Iskuskov Apr 21, 2021

Choose a reason for hiding this comment

kkonstantine commented Jul 9, 2021

Iskuskov commented Jul 9, 2021

kkonstantine left a comment • edited Loading

Choose a reason for hiding this comment

kkonstantine left a comment

Choose a reason for hiding this comment

Iskuskov commented Nov 2, 2020 •

edited

Loading

kkonstantine left a comment •

edited

Loading