Skip to content

KAFKA-4929: Transformation Key/Value type references should be to class name(), not canonicalName()#2720

Closed
bruce-szalwinski wants to merge 2 commits intoapache:trunkfrom
CDKGlobal:transforms
Closed

KAFKA-4929: Transformation Key/Value type references should be to class name(), not canonicalName()#2720
bruce-szalwinski wants to merge 2 commits intoapache:trunkfrom
CDKGlobal:transforms

Conversation

@bruce-szalwinski
Copy link
Copy Markdown
Contributor

@bruce-szalwinski bruce-szalwinski commented Mar 21, 2017

Changing getCanonicalName() references to getName() so that docs update with "$" instead of ".". Also added a connect-plugin-discovery.sh CLI to list all of the transformations available.

…renced using OuterClass$Key and OuterClass$Value.
@asfbot
Copy link
Copy Markdown

asfbot commented Mar 21, 2017

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

@asfbot
Copy link
Copy Markdown

asfbot commented Mar 21, 2017

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

@asfbot
Copy link
Copy Markdown

asfbot commented Mar 21, 2017

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

@guozhangwang
Copy link
Copy Markdown
Contributor

cc @gwenshap

@gwenshap
Copy link
Copy Markdown
Contributor

Huge props for making the error message and doc output include the right description. Current situation is very misleading.

Few comments:

  1. The new CLI is exposing what looks to me like an internal method to users. I'm all pro helping discovery, but this may need a mailing list discussion / vote. @ewencp?
  2. It will be nice to add few words about the new CLI to docs/connect.html. There's currently literally no description on what this does, how to use it, etc.
  3. Can you post a sample output of the log messages in the JIRA, so people can see "before/after"?

Other than that, the change looks small and safe so +1, pending the possible short KIP for the new CLI and adding the docs. I'll also be happy to get this in if you remove the CLI and submit it in separate PR since it may require additional process. We can fix the errors faster this way.

@bruce-szalwinski
Copy link
Copy Markdown
Contributor Author

Thanks. The SMTs are pretty cool. I've removed the connect-plugin-discovery.sh from this PR.

@asfbot
Copy link
Copy Markdown

asfbot commented Mar 22, 2017

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

@asfbot
Copy link
Copy Markdown

asfbot commented Mar 22, 2017

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

@asfbot
Copy link
Copy Markdown

asfbot commented Mar 22, 2017

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

@gwenshap
Copy link
Copy Markdown
Contributor

Tested by generating the docs. LGTM. Thank you for fixing this.

@asfgit asfgit closed this in 763ea5a Mar 22, 2017
gwenshap pushed a commit to gwenshap/kafka that referenced this pull request Apr 6, 2017
…ss name(), not canonicalName()

Changing getCanonicalName() references to getName() so that docs update with "$" instead of ".".  Also added a connect-plugin-discovery.sh CLI to list all of the transformations available.

Author: Bruce Szalwinski <bruce.szalwinski@cdk.com>

Reviewers: Gwen Shapira

Closes apache#2720 from bruce-szalwinski/transforms and squashes the following commits:

ec3b5b9 [Bruce Szalwinski] remove connect-plugin-discovery.  will submit in a different PR
eba0af7 [Bruce Szalwinski] Key / Value transformations are static nested classes and so are referenced using OuterClass$Key and OuterClass$Value.

(cherry picked from commit 763ea5a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants