Skip to content

KAFKA-9083: Various fixes/improvements for Connect's Values class#7593

Merged
rhauch merged 2 commits into
apache:trunkfrom
C0urante:kafka-9083
Jan 21, 2020
Merged

KAFKA-9083: Various fixes/improvements for Connect's Values class#7593
rhauch merged 2 commits into
apache:trunkfrom
C0urante:kafka-9083

Conversation

@C0urante

@C0urante C0urante commented Oct 24, 2019

Copy link
Copy Markdown
Contributor

Jira

The following functional changes are implemented here:
• Top-level strings beginning with "true", "false" and then followed by token delimiters (e.g., "true}" and "false]") are parsed as strings, not as booleans
• The empty array ("[]") is now parsed as an array with a null value schema
• The empty map ("{}") is now parsed as a map with null key and value schemas
• Arrays with all-null elements are now parsed successfully (whereas before an NPE was thrown) as an array with a null value schema
• Maps with all-null values are now parsed as maps with null value schemas, but non-null key schemas
• Strings that appear to be arrays at first but are missing comma delimiters (e.g., "[0 1 2]") are now parsed as strings instead of arrays
• A small improvement is made to the debug message generated when map parsing fails due to unexpected comma delimiters ("Unable to parse a map entry has no key or value" is changed to "Unable to parse a map entry with no key or value")
• A small improvement is made to the debug message generated when map parsing fails due to a missing } ("Map is missing terminating ']'" is changed to "Map is missing terminating '}'")
• A small improvement is made to the debug message generated when array or map parsing fails and parsing is reset to process the input as a string ("Unable to parse the value as a map" is changed to "Unable to parse the value as a map or an array")
• Embedded values that lack surrounding quotes (e.g., foo in "[foo]") are no longer treated as strings; this is in line with the JSON-like representation that is meant to be supported by the Values class and prevents errors such as parsing "[0 1 2]" as an array containing a single string element with a value of "0 1 2"
• The top-level string "null" is now parsed as null instead of the string "null"; this does not break attempts to convert the top-level string "null" into a string (which should also be the string "null")

Every change (except for log message alterations) is verified with one or more unit tests, and several unit tests are also added to prevent regression in functionality that, while not currently broken, is subtle enough that it may be missed in future changes without tests.

Committer Checklist (excluded from commit message)

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

@C0urante

Copy link
Copy Markdown
Contributor Author

@gharris1727 @kkonstantine would you mind taking a look when you have a chance?

@gharris1727 gharris1727 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@C0urante These look like some really important fixes, and I'm always surprised when small issues like these stick around for so long. Thanks for handling these!

I just had a few questions about parsing control flow & restructuring.

Comment thread connect/api/src/main/java/org/apache/kafka/connect/data/Values.java
Comment thread connect/api/src/main/java/org/apache/kafka/connect/data/Values.java Outdated
Comment thread connect/api/src/main/java/org/apache/kafka/connect/data/Values.java Outdated
Comment thread connect/api/src/main/java/org/apache/kafka/connect/data/Values.java
Comment thread connect/api/src/main/java/org/apache/kafka/connect/data/Values.java
Comment thread connect/api/src/main/java/org/apache/kafka/connect/data/Values.java

@gharris1727 gharris1727 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @C0urante for humoring my boolean logic and control flow nits :)

@rhauch rhauch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, @C0urante.

@rhauch rhauch merged commit 58a0b99 into apache:trunk Jan 21, 2020
@C0urante C0urante deleted the kafka-9083 branch January 21, 2020 19:37
rhauch pushed a commit that referenced this pull request Jan 21, 2020
)

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Greg Harris <gregh@confluent.io>, Randall Hauch <rhauch@gmail.com>
rhauch pushed a commit that referenced this pull request Jan 21, 2020
)

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Greg Harris <gregh@confluent.io>, Randall Hauch <rhauch@gmail.com>
rhauch pushed a commit that referenced this pull request Jan 21, 2020
)

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Greg Harris <gregh@confluent.io>, Randall Hauch <rhauch@gmail.com>
ijuma added a commit to confluentinc/kafka that referenced this pull request Jan 23, 2020
* apache-github/trunk:
  KAFKA-9418; Add new sendOffsetsToTransaction API to KafkaProducer (apache#7952)
  KAFKA-7273 Clarification on mutability of headers passed to Converter#fromConnectData() (apache#7489)
  MINOR: Only update a request's local complete time in API handler if unset (apache#7813)
  KAFKA-9143: Log task reconfiguration error only when it happened (apache#7648)
  MINOR: Change the log level from ERROR to DEBUG when failing to get plugin loader for connector (apache#7964)
  KAFKA-9024: Better error message when field specified does not exist (apache#7819)
  KAFKA-7204: Avoid clearing records for paused partitions on poll of MockConsumer (apache#7505)
  KAFKA-9083: Various fixes/improvements for Connect's Values class (apache#7593)
  MINOR: log error message from Connect sink exception (apache#7555)
qq619618919 pushed a commit to qq619618919/kafka that referenced this pull request May 12, 2020
…ache#7593)

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Greg Harris <gregh@confluent.io>, Randall Hauch <rhauch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants