KAFKA-4607: Validate the names of auto-generated internal topics by nixsticks · Pull Request #2331 · apache/kafka

nixsticks · 2017-01-06T23:27:22Z

I considered catching errors to add further information about naming internal state stores. However, Topic.validate() will throw an error that prints the offending name, so I decided not to add too much complexity.

asfbot · 2017-01-06T23:29:32Z

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

mjsax · 2017-01-06T23:48:48Z

I think it is better to add this check closer to the user to avoid deep stack traces. Thus, I would add it to KGroupedStreamImpl and KGroupedKTabelImpl.

\cc @guozhangwang @dguy @enothereska

mjsax · 2017-01-06T23:50:38Z

Furthermore, can you please add a test for this. And please update the corresponding JavaDocs and state that store names but be valid topic names (and explain what this means, ie, which characters are allowed and which not). Thx.

asfbot · 2017-01-07T00:02:46Z

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

nixsticks · 2017-01-07T00:32:36Z

Will look into it. Thanks @mjsax !

asfbot · 2017-01-07T00:46:28Z

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

nixsticks · 2017-01-07T22:34:50Z

@mjsax Do you think it might be good to put it both in KGroupedStreamImpl/KGroupedTableImpl and also in InternalTopicConfig in case other, new code tries to use InternalTopicConfig as well?

mjsax · 2017-01-07T22:45:39Z

Sounds like a good idea to me. Maybe we can put it as an assertion into InternalTopicConfig ?

nixsticks · 2017-01-09T19:23:53Z

I did a couple of cleanups like this in the cases where it seemed like the test name did not match the actual test, since I am technically writing and editing tests regarding the state store names -- see also shouldNotAcceptNullStateStoreSupplierWhenAggregatingSessionWindows and shouldNotAcceptNullStateStoreSupplierWhenCountingSessionWindows

The basic rule is to have a single PR for a single JIRA -- changes like this add "noise" and complicate reviewing in general (review need to know/judge what is part of the actual PR and what is cleanup). For this case it is ok though IMHO :)

nixsticks · 2017-01-09T19:27:14Z

@mjsax There are now ... a lot of changes. I am fairly new to contributing to open source so please let me know if I have updated the pull request in the wrong way. I left the full validation in InternalTopicConfig since I figured that internal topics should also follow the rules of regular topics.

nixsticks · 2017-01-09T19:27:38Z

This was taken from the Scala version of this utility class and I am not sure what best to call it

That's a tricky one... we do not want to have code duplication -- but we also do not want to depend on kafka-core module...
\cc @guozhangwang What do you think?

Should we change the scala version to use this one in common? i believe kafka-core depends on common. It would be better to not have the code duplicated if we can avoid @ijuma

Yes, if we move the logic here, then we should reuse it and not duplicate it (including tests if they exist). A couple more things:

This class should be in common.internals since we it's an internal class.

We are doing this validation client-side, which could potentially be different from the broker validation (e.g. a future version could allow longer topic names or restrict it further due to file system or OS restrictions). It would be good to explain what is the current behaviour to make it clear how this improves on that.

asfbot · 2017-01-09T20:09:57Z

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

asfbot · 2017-01-09T20:20:07Z

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

asfbot · 2017-01-09T20:38:05Z

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

mjsax

Thanks for the update. I have some comment. Overall look already quite good. Fee free to wait with an update until @guozhangwang replied.

mjsax · 2017-01-09T21:15:45Z

Please add this to StatsStoreSupplier#name(), too? (plus add a test for it)

mjsax · 2017-01-09T21:19:11Z

Nit: we try to follow the following policy to make reviewing easier:

each new sentence start a new line (you do follow this already)

no line longer then 120 character (otherwise lines are longer then Github displays and you need to scroll to the right)

mjsax · 2017-01-09T21:21:35Z

This adds code redundancy. Please remove. Same blow. Only add validation to the already present not-null checks.

mjsax · 2017-01-09T21:23:54Z

Remove: same as above.

mjsax · 2017-01-09T21:24:26Z

Again. Remove.

mjsax · 2017-01-09T21:26:17Z

Use a class member and use in all test methods to avoid code duplication.
Please add final keyword wherever possible.

mjsax · 2017-01-09T21:27:55Z

Use class final class member.

mjsax · 2017-01-09T21:29:02Z

That's a tricky one... we do not want to have code duplication -- but we also do not want to depend on kafka-core module...
\cc @guozhangwang What do you think?

miguno · 2017-01-24T10:54:05Z

What's the status of this PR? I'd be great to get it merged soon.

asfbot · 2017-01-24T11:34:13Z

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

asfbot · 2017-01-24T12:07:13Z

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

asfbot · 2017-01-24T12:35:37Z

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

guozhangwang · 2017-01-24T17:44:37Z

Seems @nixsticks has not addressed the comments yet.

guozhangwang

A meta comment: instead of checking validity in the builder, could we just check validity once in the InternalTopicManager, before calling StreamsKafkaClient to create them, just like what we did in the scala's AdminUtils pattern?

The javadocs addition looks good to me.

mjsax · 2017-01-24T19:13:43Z

@guozhangwang I prefer shallow stack traces and thus IMHO we should check as soon as we can if names are valid. (Of course, just my personal opinion.)

guozhangwang · 2017-01-24T20:02:23Z

@guozhangwang I prefer shallow stack traces and thus IMHO we should check as soon as we can if names are valid. (Of course, just my personal opinion.)

I'm convinced.

nixsticks · 2017-02-03T16:13:52Z

@guozhangwang Sorry, I was going to wait for an update on the kafka-core module vs code duplication, and then I went on PTO for two weeks! I will try to get my changes in soon (I'm returning to the US in a few days), but before that, would you mind addressing the comment from @mjsax about the above?

nixsticks · 2017-02-03T17:59:52Z

Not sure what happened to my previous comment, but: I ended up addressing most of the comments here tonight. Could you let me know whether the changes are acceptable, and what you'd like regarding the addition of Topic.java vs using the kafka-core module? Thanks!

asfbot · 2017-02-03T18:48:16Z

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

asfbot · 2017-02-03T18:52:37Z

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

asfbot · 2017-03-15T02:22:50Z

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

asfbot · 2017-03-15T02:23:04Z

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

asfbot · 2017-03-15T02:24:13Z

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

mjsax · 2017-03-15T16:03:47Z

@nixsticks There was a checkstyle error in the build. Can you have a look. Thx.

nixsticks · 2017-03-15T16:21:05Z

@mjsax I did, last night, but it is for a file (ByteArrayConverter on Connect) that I did not touch.

checkstyleMain[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk8-scala2.11/connect/runtime/src/main/java/org/apache/kafka/connect/converters/ByteArrayConverter.java:1: Line does not match expected header line of '/*'. [Header]

mjsax · 2017-03-15T16:39:23Z

Try to rebase to trunk

guozhangwang · 2017-03-15T17:14:11Z

@mjsax I did, last night, but it is for a file (ByteArrayConverter on Connect) that I did not touch.

I think this is not from your code, but another PR merged recently that has a header issue, and it has been fixed in a most recent commit: 962c378

asfbot · 2017-03-15T17:27:18Z

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

asfbot · 2017-03-15T17:27:25Z

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

asfbot · 2017-03-15T17:28:13Z

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

nixsticks · 2017-03-15T17:40:40Z

@mjsax @guozhangwang Rebased

mjsax

LGTM.

nixsticks · 2017-03-15T22:32:00Z

BTW, what is the generally followed process for squashing/rebasing commits before merging?

mjsax · 2017-03-16T00:42:40Z

We don't have an official policy. But this are my two cents:

As long as you work on the PR, it's best to just append new commits. This simplifies review process as the reviewer can inspect the deltas
If rebasing is required (eg., github shows some merge conflicts), just rebase (if possible don't squash commits -- but of course, if you squash first and rebase then, rebasing is simpler and it's also ok if you do this)

When a PR get's merged, the committer doing the merge will squash all commits anyway (so you don't need to squash by yourself). The commit message will contain the github title and github description for the PR (IRRC).

So overall, you don't need to worry about it too much :)

ijuma · 2017-03-16T01:08:16Z

The details can be found in the following wiki page:

https://cwiki.apache.org/confluence/display/KAFKA/Contributing+Code+Changes

What @mjsax is mostly right (apart from the no official policy comment :)). I'd just git merge instead of git rebase when you intend to update the branch with changes from trunk. Since commits are squashed automatically, there's not much point in using rebase and it doesn't make clear when the branch was updated to include more trunk commits.

ijuma · 2017-03-16T01:18:34Z

By the way, one issue I raised early on is that doing this client-side won't necessarily work in every case. Here's a concrete example:

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

Probably still worthwhile doing it, but we still need to handle errors by the broker if it's stricter for whatever reason.

mjsax · 2017-03-16T17:07:48Z

@ijuma Thanks for clarification! :)

About KAFKA-4893 -- I guess, we should add a check to Streams, but this would be a different PR. The store names checked here are only part of topic names, and thus, we cannot check the topic name length at this level. I created https://issues.apache.org/jira/browse/KAFKA-4912 for this.

guozhangwang

Merged to trunk.

nixsticks force-pushed the internal-topics branch from 5ffb1cc to 2f2d7c3 Compare January 9, 2017 19:22

nixsticks commented Jan 9, 2017

View reviewed changes

nixsticks force-pushed the internal-topics branch from 2f2d7c3 to e664456 Compare January 9, 2017 19:29

mjsax requested changes Jan 9, 2017

View reviewed changes

guozhangwang reviewed Jan 24, 2017

View reviewed changes

nixsticks force-pushed the internal-topics branch from 852f24c to 0cbccc7 Compare February 3, 2017 17:28

nixsticks added 10 commits March 15, 2017 12:42

KAFKA-4607: Validate the names of auto-generated internal topics

d77d393

Throw exception closer to user, add tests

0d75f1f

Addressing further comments

79aab6e

KAFKA-4607: Validate the names of auto-generated internal topics

2e8565d

Throw exception closer to user, add tests

da3cb4c

Addressing further comments

f1d8706

Address minor comments, move Topic into o.a.k.common.internals

5394acb

Fix checkstyle errors

b7290d0

Address comments, add more tests

5fd7e7a

Sigh

427c72b

nixsticks force-pushed the internal-topics branch from e2ec08f to 427c72b Compare March 15, 2017 16:42

mjsax approved these changes Mar 15, 2017

View reviewed changes

guozhangwang approved these changes Mar 17, 2017

View reviewed changes

asfgit closed this in 9e78771 Mar 17, 2017

Conversation

nixsticks commented Jan 6, 2017

Uh oh!

asfbot commented Jan 6, 2017

Uh oh!

mjsax commented Jan 6, 2017

Uh oh!

mjsax commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfbot commented Jan 7, 2017

Uh oh!

nixsticks commented Jan 7, 2017

Uh oh!

asfbot commented Jan 7, 2017

Uh oh!

nixsticks commented Jan 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjsax commented Jan 7, 2017

Uh oh!

nixsticks Jan 9, 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!

nixsticks commented Jan 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 9, 2017

Uh oh!

asfbot commented Jan 9, 2017

Uh oh!

asfbot commented Jan 9, 2017

Uh oh!

mjsax left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miguno commented Jan 24, 2017

Uh oh!

asfbot commented Jan 24, 2017

Uh oh!

asfbot commented Jan 24, 2017

Uh oh!

asfbot commented Jan 24, 2017

Uh oh!

guozhangwang commented Jan 24, 2017

Uh oh!

guozhangwang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mjsax commented Jan 6, 2017 •

edited

Loading

nixsticks commented Jan 7, 2017 •

edited

Loading

nixsticks Jan 9, 2017 •

edited

Loading

guozhangwang left a comment •

edited

Loading

nixsticks commented Feb 3, 2017 •

edited

Loading

nixsticks commented Feb 3, 2017 •

edited

Loading

nixsticks commented Mar 15, 2017 •

edited

Loading

nixsticks commented Mar 15, 2017 •

edited

Loading