KAFKA-4709：Error message from Struct.validate() should include the name of the offending field. by Aegeaner · Pull Request #2521 · apache/kafka

Aegeaner · 2017-02-09T08:34:05Z

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

asfbot · 2017-02-09T08:36:09Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1583/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-02-09T08:37:58Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1586/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-02-09T08:38:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1583/
Test FAILed (JDK 7 and Scala 2.10).

lindong28 · 2017-02-12T17:52:24Z

+            try {
+                ConnectSchema.validateValue(fieldSchema, value);
+            } catch(DataException e) {
+                throw new DataException("Invalid value: null used for required field: "  + field.name() + ", schema type: " + fieldSchema.type());


While current implementation of validateValue only throws DataException when a required field has null value, in general we may have other exception in the future, right? Then it may be misleading to claim that "null used for required field" here. Maybe you can do throw new DataException(..., e) here.

yes, I will keep the original exception message and add some additional field info on it.

cc @lindong28

asfbot · 2017-02-13T02:48:20Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1651/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-02-13T02:48:23Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1654/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-02-13T02:50:08Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1651/
Test FAILed (JDK 7 and Scala 2.10).

…o on it

asfbot · 2017-02-13T02:54:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1652/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-02-13T02:54:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1655/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-02-13T02:56:35Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1652/
Test FAILed (JDK 7 and Scala 2.10).

lindong28 · 2017-02-13T03:41:17Z

+            try {
+                ConnectSchema.validateValue(fieldSchema, value);
+            } catch(DataException e) {
+                throw new DataException("Validate failed for required field: \""  + field.name() + "\", "


Strictly speaking you don't know whether this is a required field here. Also, you probably don't need to print e.getMessage() again since the exception have already included this message by including e in the newly constructed exception.

Actually, if all you need is the field name, would it be simpler to just keep the Struct.java as it is, and change ConnectSchema.java to do throw new DataException("Invalid value: null used for required field " + schema.name())

this is also my first intuition here, but schema.name() returns null here. And also, schema name is not field name.

Throwing new DataException(..., e) doesnet include original exception message, but only set up original exception as the cause in new exception. @lindong28

@Aegeaner Yeah the original exception message is included as cause and will be printed to the log or console when the exception is thrown. That is why I think the 2nd exception includes the original exception message. I thought this is enough for user.

Also, I see your point that schema.name() is null. Would it be more precise to remove required from the 2nd exception message? Another alternative is to include an identifier (e.g. schema field name) as a parameter to validateValue(...) so that this method can identify the field with problem in the exception message. Doing this allows us to identify the field in all scenarios where validateValue(...) failed. But this also requires more change so you may want to do this only if a committer suggested so.

@lindong28 yeah validateValue(..) method is more primitive so not that easy to change

cc @guozhangwang

asfbot · 2017-02-13T04:36:34Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1654/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-02-13T04:36:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1657/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-02-13T04:38:57Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1654/
Test FAILed (JDK 7 and Scala 2.10).

guozhangwang · 2017-02-14T07:33:15Z

@Aegeaner @lindong28 I'd suggest change the signature of validateValue to take Field and Object as the parameter. BTW is this a public API? cc @ewencp

Aegeaner · 2017-02-14T10:17:22Z

@guozhangwang It's not a public API but is used quite often in connect schema tests. Kafka Connect exposed REST api to validate configs.

guozhangwang · 2017-02-14T15:53:26Z

@Aegeaner I understand. If possible we an just change its usage in all the unit test as well.

lindong28 · 2017-02-14T17:11:20Z

@guozhangwang @Aegeaner I am not sure if we can simply change the method signature to be validateValue(Field field, Object value). Suppose schema.type is ARRAY and validateValue(...) calls itself recursively, what should be the value of field? What if schema.type is MAP? Maybe we should just add a method with signature validateValue(String name, Schema schema, Object value). The code change would be much smaller with this approach. The existing validateValue(Schema schema, Object value) can just call validateValue(null, schema, value) so that we don't have to touch unit tests.

guozhangwang · 2017-02-14T17:22:34Z

@lindong28 Sounds good to me.

Aegeaner · 2017-02-15T03:25:49Z

@lindong28 @guozhangwang Thanks for your advice. I have changed validateValue(..) signature to pass the field name.

lindong28 · 2017-02-15T03:43:02Z

        if (value == null) {
            if (!schema.isOptional())
-                throw new DataException("Invalid value: null used for required field");
+                throw new DataException("Invalid value null used for required field: \"" + name


nits: not sure if we should : after Invalid value in the error message. Otherwise LGTM.

Thanks for the update.

@lindong28 Thanks, I have added field info to all DataException message in validateValue(..).

asfbot · 2017-02-15T04:08:20Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1691/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-02-15T04:40:37Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1688/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-02-15T06:23:58Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1688/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-02-15T07:11:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1692/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-02-15T07:11:59Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1689/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-02-15T07:13:03Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1689/
Test FAILed (JDK 7 and Scala 2.10).

lindong28 · 2017-02-15T18:37:21Z


        if (expectedClasses == null)
-            throw new DataException("Invalid Java object for schema type " + schema.type() + ": " + value.getClass());
+            throw new DataException("Invalid Java object for schema type " + schema.type()


The error message is updated to say "required field". Do we actually know that this is a required field here?

lindong28 · 2017-02-15T18:37:53Z

        }
        if (!foundMatch)
-            throw new DataException("Invalid Java object for schema type " + schema.type() + ": " + value.getClass());
+            throw new DataException("Invalid Java object for schema type " + schema.type()


The error message is updated to say "required field". Do we actually know that this is a required field here?

I have removed "required" from error message. we can judge if it's required field from schema.isOptional() , but we may don't care about it when the value is not null.

guozhangwang · 2017-02-15T21:09:19Z

Added FakeSchema does not have apache header, hence the checkstyle error:

scala2.12/connect/api/src/test/java/org/apache/kafka/connect/data/FakeSchema.java:1: Line does not match expected header line of '/\*\*'.
 FAILED

asfbot · 2017-02-16T03:33:26Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1704/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-02-16T03:39:25Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1701/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-02-16T03:59:49Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1701/
Test FAILed (JDK 8 and Scala 2.12).

guozhangwang · 2017-02-16T05:34:22Z

looks good to me overall. @lindong28 do you want to take another look?

lindong28 · 2017-02-16T18:59:44Z

@guozhangwang @Aegeaner Thank you. LGTM.

guozhangwang · 2017-02-16T21:48:19Z

Merged to trunk.

…me of the offending field. https://issues.apache.org/jira/browse/KAFKA-4709 Author: Aegeaner <xihuke@gmail.com> Reviewers: Dong Lin, Guozhang Wang Closes apache#2521 from Aegeaner/KAFKA-4709

KAFKA-4709

dce515e

lindong28 reviewed Feb 12, 2017

View reviewed changes

keep the original exception message and add some additional field inf…

1465cf4

…o on it

lindong28 reviewed Feb 13, 2017

View reviewed changes

no getmessage call on original exception

2380425

change validateValue(..) signature to pass field name

24d6ea3

lindong28 reviewed Feb 15, 2017

View reviewed changes

Add field info to all DataException message

b6c3156

lindong28 reviewed Feb 15, 2017

View reviewed changes

add lisence header

5fbff06

asfgit closed this in 6c83939 Feb 16, 2017

Conversation

Aegeaner commented Feb 9, 2017

Uh oh!

asfbot commented Feb 9, 2017

Uh oh!

asfbot commented Feb 9, 2017

Uh oh!

asfbot commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Feb 13, 2017

Uh oh!

asfbot commented Feb 13, 2017

Uh oh!

asfbot commented Feb 13, 2017

Uh oh!

asfbot commented Feb 13, 2017

Uh oh!

asfbot commented Feb 13, 2017

Uh oh!

asfbot commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lindong28 Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Feb 13, 2017

Uh oh!

asfbot commented Feb 13, 2017

Uh oh!

asfbot commented Feb 13, 2017

Uh oh!

guozhangwang commented Feb 14, 2017

Uh oh!

Aegeaner commented Feb 14, 2017

Uh oh!

guozhangwang commented Feb 14, 2017

Uh oh!

lindong28 commented Feb 14, 2017

Uh oh!

guozhangwang commented Feb 14, 2017

Uh oh!

Aegeaner commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Feb 15, 2017

Uh oh!

asfbot commented Feb 15, 2017

Uh oh!

asfbot commented Feb 15, 2017

Uh oh!

asfbot commented Feb 15, 2017

Uh oh!

asfbot commented Feb 15, 2017

Uh oh!

asfbot commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lindong28 Feb 13, 2017 •

edited

Loading