KAFKA-2832: Add a consumer config option to exclude internal topics #932

vahidhashemian · 2016-02-18T16:26:57Z

A new consumer config option 'exclude.internal.topics' was added to allow excluding internal topics when wildcards are used to specify consumers.
The new option takes a boolean value, with a default 'false' value (i.e. no exclusion).

This patch is co-authored with @rajinisivaram.

vahidhashemian · 2016-02-19T09:57:15Z

@granthenke Thanks for your review, and fair comments. I'll update the patch.

granthenke · 2016-02-19T14:48:04Z

I would be interested in what others think about the default value.

@rajinisivaram Mentioned on my comment on the old commit here that default behavior for the new consumer in 0.9.0.0 and 0.9.0.1 is false because excluding the internal topics did not exist. Is that behavior we need to maintain or can we set the default to true for consistency and to cover the common case?

granthenke · 2016-03-01T22:53:04Z

@ewencp @ijuma @gwenshap Pinging for review.

ijuma · 2016-03-02T12:17:37Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java

+    private final boolean excludeInternalTopics;
+
+    static {
+        INTERNAL_TOPICS.add("__consumer_offsets");


One issue with this is that older clients are meant to work with newer brokers and if additional internal topics are added, this could get out of sync.

Not sure if it's a problem in practice, but I thought I'd mention it.

@ijuma Thats a really good point. The admin scripts have checks for "Internal Topics" (example) too. As a part of KIP-4 I may need to add a boolean field to the MetadataResponse indicating if a topic is internal so I can support all existing tool functionality. Perhaps this could use that too?

Yeah, that would be a nicer way to do it.

@ijuma @granthenke Thank you for the feedback. What I understand is we should wait for KIP-4 to merge so we can leverage the boolean field here.

@granthenke Would you please make the JIRA ticket for this PR dependent on the proper sub-task of KIP-4 (that would implement the boolean field you mentioned)? That way I can update this PR as soon as the KIP-4 sub-task is finished. Thanks.

ijuma · 2016-03-02T12:21:51Z

cc @hachikuji

hachikuji · 2016-03-02T18:43:18Z

clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java

@@ -172,6 +172,10 @@
    public static final String MAX_POLL_RECORDS_CONFIG = "max.poll.records";
    private static final String MAX_POLL_RECORDS_DOC = "The maximum number of records returned in a single call to poll().";

+    /** <code>exclude.internal.topics</code> */
+    public static final String EXCLUDE_INTERNAL_TOPICS_CONFIG = "exclude.internal.topics";
+    private static final String EXCLUDE_INTERNAL_TOPICS_DOC = "Whether messages from internal topics (such as offsets) should be exposed to the consumer.";


Can you clarify this description a little bit? It seems like it's still possible for the user to subscribe to internal topics explicitly. Also, don't we still return internal topics from the listTopics() API?

@hachikuji I actually copied the description from the old consumer config (here) to remain consistent. If "true" internal topics will not be exposed to the new consumer; if "false" they will.

With respect to listTopic() API nothing is changed, which is, if I'm not mistaken, internal topics are still returned.

I missed the part about "messages," so yeah, no confusion about the listTopics() API. I think it still may be worth clarifying that messages from internal topics will only be included if those topics are explicitly subscribed to.

One other minor note: for whatever reason, the new consumer uses the term "records" instead of "messages." We flop back and forth between these terms even in this class, but it would be nice to start to settle.

@hachikuji Sure, I'll update the text, and mention that if this config is "true" the only way to receive messages from internal topics is subscribing to them. Also, I'll replace references to "message" with "record". Thanks for the feedback.

ewencp · 2016-03-09T07:01:24Z

Did the necessary updates get completed here?

Re: default, I think we can view this as buggy behavior that nobody should be relying on, although I guess it could cause a bit of compatibility pain since the flag didn't previously exist. But the unexpected results of, e.g., consumer offsets being caught by MM is probably much worse.

@ijuma Is this possibly a blocker for 0.10 instead of just major? Seems like a pretty important fix to get in now, before adoption of the new consumer really picks up.

ijuma · 2016-03-09T09:42:02Z

@ewencp I marked it as a blocker for now. What are your thoughts on the hard-coded approach used here versus getting the data from the server via the topic metadata?

ewencp · 2016-03-09T21:16:35Z

@ijuma Getting the list via topic metadata is definitely the right solution. If it depends on some other patch, the hard coded approach might be a good solution to address the immediate problem, and a follow up could always fill in the complete fix.

granthenke · 2016-03-09T21:17:36Z

@ewencp @ijuma Agreed. Lets get a fix in and then follow up later once the metadata changes are made.

vahidhashemian · 2016-03-09T21:26:58Z

I'll submit an update shortly to take care of the other comments on the PR.

gwenshap · 2016-03-09T22:47:35Z

Sorry if this was already discussed - but we have Topics.internalTopics which already contains a list of internal topics and it is used in about 3 places.

We probably don't want to duplicate this information and since we can't have the new consumer depend on the scala objects, can we move this definition to o.a.k.common somewhere and refactor the scala code to use this list?

Even if we can't use it in the server yet, we probably want to push the list outside the internal AbstractCoordinator and into somewhere in Common package, so it will be visible for reuse in future client-side tools (KIP-4 comes to mind...)

ijuma · 2016-03-09T22:51:57Z

Sounds good to me @gwenshap.

@rajinisivaram

A new consumer config option 'exclude.internal.topics' was added to allow excluding internal topics when wildcards are used to specify consumers. The new option takes a boolean value, with a default 'false' value (i.e. no exclusion). This patch is co-authored with @rajinisivaram.

vahidhashemian · 2016-03-10T17:39:42Z

@gwenshap @granthenke @ijuma @ewencp Thanks for your feedback. I updated the patch according to @gwenshap's suggestion. I hope the update is close to what she had in mind.

gwenshap · 2016-03-10T18:05:37Z

I actually thought of something less extensive... I didn't intend to move GroupMetadataTopicName (although that also looks ok to me), what I did intend is to get rid of Topic.InternalTopics.
Is there any reason to keep it around now that CommonDefs.INTERNAL_TOPICS exist?

granthenke · 2016-03-10T18:10:33Z

clients/src/main/java/org/apache/kafka/common/CommonDefs.java

+public final class CommonDefs {
+    // TODO: we store both group metadata and offset data here despite the topic name being offsets only
+    public static final String GROUP_METADATA_TOPIC_NAME = "__consumer_offsets";
+    public static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(new HashSet<String>(Arrays.asList(GROUP_METADATA_TOPIC_NAME)));


Do we really want anything related to internal topics to be client side? This could change in brokers from version to version and the clients should still work. I understand that for now we have no way to get that information, but we will soon (KAFKA-3306). I imagine removing the client side list would be part of the cleanup once thats available. So whatever exists in the mean time should be private so we don't need a deprecation cycle.

Yes, this should on an internal package (eg common.internals).

@ijuma I assume you mean moving the whole class under common.internals. I can do that.

@granthenke Sorry for the naive question. INTERNAL_TOPICS is being used in ConsumerCoordinator; so making it private would break how it's currently being used there. Making it private could also cause issues with implementing @gwenshap's comment to remove Topic.InternalTopics and use this new INTERNAL_TOPICS instead. Could you please clarify? Thanks.

@vahidhashemian, yes, that's what I mean.

@gwenshap meant that kafka.common.Topic.InternalTopics should be removed in favour of the INTERNAL_TOPICS defined in this PR.

vahidhashemian · 2016-03-10T18:38:10Z

@gwenshap You are right. We can remove Topic.InternalTopics and make use of CommonDefs.INTERNAL_TOPICS instead.

ijuma · 2016-03-15T09:46:35Z

clients/src/main/java/org/apache/kafka/common/CommonDefs.java

+/**
+ * Common definitions used in client-side tools
+ */
+public final class CommonDefs {


Instead of the overly generic CommonDefs, why not call this Topics or something like that?

edoardocomar · 2016-03-16T15:06:32Z

Hi all, Vahid is on vacation so I am creating a new pull request #1082 to accommodate the changes discussed so far - and added a unit test for the new option.
PR #1082 supersedes this one.

vahidhashemian · 2016-04-14T22:05:46Z

PR #1082 resumes this PR.

jeffwidman · 2016-09-26T19:56:10Z

For anyone else reading this later, the default is to IGNORE internal topics, not include them (contrary to what the top comment says).

vahidhashemian force-pushed the KAFKA-2832 branch from 558e49a to ca4c47f Compare February 19, 2016 09:58

vahidhashemian closed this Feb 19, 2016

vahidhashemian reopened this Feb 19, 2016

vahidhashemian force-pushed the KAFKA-2832 branch from ca4c47f to 1b03ad9 Compare February 23, 2016 20:20

vahidhashemian closed this Feb 23, 2016

vahidhashemian reopened this Feb 23, 2016

vahidhashemian force-pushed the KAFKA-2832 branch from 1b03ad9 to 8829be1 Compare February 24, 2016 00:10

ijuma reviewed Mar 2, 2016
View reviewed changes

hachikuji reviewed Mar 2, 2016
View reviewed changes

vahidhashemian force-pushed the KAFKA-2832 branch 3 times, most recently from 220dce0 to 0578214 Compare March 9, 2016 21:50

vahidhashemian force-pushed the KAFKA-2832 branch from 0578214 to 09628f3 Compare March 10, 2016 01:41

vahidhashemian force-pushed the KAFKA-2832 branch from 09628f3 to c054153 Compare March 10, 2016 01:43

granthenke reviewed Mar 10, 2016
View reviewed changes

ijuma reviewed Mar 15, 2016
View reviewed changes

edoardocomar mentioned this pull request Mar 16, 2016

KAFKA-2832: Add a consumer config option to exclude internal topics #1082

Closed

vahidhashemian closed this Apr 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-2832: Add a consumer config option to exclude internal topics #932

KAFKA-2832: Add a consumer config option to exclude internal topics #932

vahidhashemian commented Feb 18, 2016

vahidhashemian commented Feb 19, 2016

granthenke commented Feb 19, 2016

granthenke commented Mar 1, 2016

ijuma Mar 2, 2016

ijuma Mar 2, 2016

granthenke Mar 2, 2016

ijuma Mar 2, 2016

vahidhashemian Mar 2, 2016

vahidhashemian Mar 4, 2016

ijuma commented Mar 2, 2016

hachikuji Mar 2, 2016

vahidhashemian Mar 2, 2016

hachikuji Mar 2, 2016

hachikuji Mar 2, 2016

vahidhashemian Mar 2, 2016

ewencp commented Mar 9, 2016

ijuma commented Mar 9, 2016

ewencp commented Mar 9, 2016

granthenke commented Mar 9, 2016

vahidhashemian commented Mar 9, 2016

gwenshap commented Mar 9, 2016

ijuma commented Mar 9, 2016

vahidhashemian commented Mar 10, 2016

gwenshap commented Mar 10, 2016

granthenke Mar 10, 2016

ijuma Mar 10, 2016

vahidhashemian Mar 10, 2016

ijuma Mar 15, 2016

ijuma Mar 15, 2016

vahidhashemian commented Mar 10, 2016

ijuma Mar 15, 2016

edoardocomar commented Mar 16, 2016

vahidhashemian commented Apr 14, 2016

jeffwidman commented Sep 26, 2016

KAFKA-2832: Add a consumer config option to exclude internal topics #932

KAFKA-2832: Add a consumer config option to exclude internal topics #932

Conversation

vahidhashemian commented Feb 18, 2016

vahidhashemian commented Feb 19, 2016

granthenke commented Feb 19, 2016

granthenke commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma commented Mar 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewencp commented Mar 9, 2016

ijuma commented Mar 9, 2016

ewencp commented Mar 9, 2016

granthenke commented Mar 9, 2016

vahidhashemian commented Mar 9, 2016

gwenshap commented Mar 9, 2016

ijuma commented Mar 9, 2016

vahidhashemian commented Mar 10, 2016

gwenshap commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vahidhashemian commented Mar 10, 2016

Choose a reason for hiding this comment

edoardocomar commented Mar 16, 2016

vahidhashemian commented Apr 14, 2016

jeffwidman commented Sep 26, 2016