Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-4714: TimestampConverter transformation (KIP-66) #3065

Closed
wants to merge 2 commits into from

Conversation

ewencp
Copy link
Contributor

@ewencp ewencp commented May 16, 2017

No description provided.

@ewencp
Copy link
Contributor Author

ewencp commented May 16, 2017

This is a WIP built on top of #2458, so you're seeing a larger diff than is ideal. The primary addition here is the TimestampConverter transformation in the last commit (9eafd31). I'm filing this now for review of the new bits, and can re-file or rebase as needed.

@ewencp
Copy link
Contributor Author

ewencp commented May 16, 2017

/cc @kkonstantine @hachikuji @gwenshap

@hachikuji
Copy link
Contributor

@ewencp Trying to reclaim the LOC lead, eh?

@asfbot
Copy link

asfbot commented May 16, 2017

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

@ewencp
Copy link
Contributor Author

ewencp commented May 16, 2017

@hachikuji lol, removing the previous PR the diffstat is not very large. Just writing some code to keep myself sane :)

@asfbot
Copy link

asfbot commented May 16, 2017

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

Copy link
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

Reviewed only the timestamp-related additions here.

Looks good to me and tested. Left a slightly unrelated comment since we are changing that file too.

@@ -32,6 +32,7 @@ public static void requireSchema(Schema schema, String purpose) {
}
}

@SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are touching this class, I noticed use of getCanonicalName. Do we want to change to getName here too? It doesn't really matter, it's only used for logging, but I mention it for consistency.

@ewencp ewencp force-pushed the kafka-3209-timestamp-converter branch from 9eafd31 to 1e80362 Compare May 18, 2017 00:37
@asfbot
Copy link

asfbot commented May 18, 2017

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

@asfbot
Copy link

asfbot commented May 18, 2017

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

@ewencp ewencp force-pushed the kafka-3209-timestamp-converter branch from 1e80362 to 64b4ecb Compare May 18, 2017 05:00
@asfbot
Copy link

asfbot commented May 18, 2017

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

@asfbot
Copy link

asfbot commented May 18, 2017

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

@ewencp ewencp force-pushed the kafka-3209-timestamp-converter branch from 5ed1f20 to 93bad0a Compare May 19, 2017 02:49
@ewencp
Copy link
Contributor Author

ewencp commented May 19, 2017

@kkonstantine Updated, good catch. I've now grepped through the code to make sure we're not using getCanonicalName elsewhere since it isn't what we want but sounds like it -- the only remaining use is harmless.

@hachikuji If you happen to have time to review, we can probably squeeze this into 0.11.0.0 -- there's some sort of small feature provision to get things in post-feature-freeze. If not, we can always bump it to a future release, in which case I'll create yet another KIP-66 JIRA and relabel this.

@asfbot
Copy link

asfbot commented May 19, 2017

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

@asfbot
Copy link

asfbot commented May 19, 2017

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

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM

asfgit pushed a commit that referenced this pull request May 19, 2017
Author: Ewen Cheslack-Postava <me@ewencp.org>

Reviewers: Konstantine Karantasis <konstantine@confluent.io>, Jason Gustafson <jason@confluent.io>

Closes #3065 from ewencp/kafka-3209-timestamp-converter

(cherry picked from commit 61bab2d)
Signed-off-by: Jason Gustafson <jason@confluent.io>
@asfgit asfgit closed this in 61bab2d May 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants