KAFKA-3959: enforce offsets.topic.replication.factor upon __consumer_offsets auto topic creation (KIP-115) #2177
Conversation
|
@onurkaraman LGTM for the change itself. I'm guessing this might have some repercussions for system tests though. Maybe we should add the override to |
|
@hachikuji yeah agreed. I realized after submitting the PR that I hadn't even run the tests. I had only done a manual test. Config values in the integration/system tests may need to change. I can look into it. |
onurkaraman
force-pushed
the
KAFKA-3959
branch
from
ef85582
to
85a95c2
Compare
|
Force pushed a change that should fix the unit/integration tests. Still need to look into system tests. |
onurkaraman
force-pushed
the
KAFKA-3959
branch
from
85a95c2
to
db9c3fd
Compare
|
I think this is a good change. A couple of comments:
|
onurkaraman
force-pushed
the
KAFKA-3959
branch
from
db9c3fd
to
43c0c49
Compare
|
Added a note to upgrade.html and updated the config documentation in KafkaConfig. |
|
Something to consider is that internal topic creation can happen in five paths:
Consequences of this PR:
So scenario 1 has been fixed, 2 has an unexpected change, and 3-5 remain unchanged but potentially do the wrong thing. |
|
6th way: By a user directly writing to the topic znode in zookeeper. |
|
That being said, I don't think we need to worry about users manually creating the __consumer_offsets topic for now, so really we just need to worry about path 1 and maybe 2. |
|
Only worrying about 1 & 2 for internal topics seems reasonable to me. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Yeah, focusing on 1 & 2 makes sense. @ijuma Feels a bit weird to require a KIP to enforce an existing configuration correctly 😉 . That said, I'm not too sure about the impact, so might be safer to do a KIP anyway. Seems this only affects new clusters, so I imagine the main impact would be broken tests. Perhaps some broken deployment recipes also? |
|
@hachikuji, sorry for the delay. I agree that this should have been the correct behaviour for the configuration. However, the previous behaviour was specified and it was not an accident either (Jun and Neha discussed it in the original JIRA). Given that and the potential impact on new deployments, a KIP seems inevitable. It should be a simple one and it will hopefully pass as easily as the unclean leader election one. :) |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
For 1, I think we all agree that it should be solved with GROUP_COORDINATOR_NOT_AVAILABLE. For 2, I think we want to preserve existing MetadataRequest topic creation behavior by returning INVALID_REPLICATION_FACTOR, but I don't know if we actually agreed on that. If this is what we want, then I need to revise the code. |
onurkaraman
force-pushed
the
KAFKA-3959
branch
from
43c0c49
to
c762404
Compare
|
I updated the PR assuming the behavior mentioned in my previous comment. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
onurkaraman
force-pushed
the
KAFKA-3959
branch
from
c762404
to
1b6d044
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
@hachikuji @ijuma @ewencp so what is the plan? Can we just check it into 0.10.2.0? I need to know the release in order to tweak the upgrade.html correctly. |
|
Refer to this link for build results (access rights to CI server needed): |
onurkaraman
force-pushed
the
KAFKA-3959
branch
from
1b6d044
to
b2b4332
Compare
|
Updated the upgrade.html so the comment is under the 0.10.2.0 notable changes. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
…offsets auto topic creation (KIP-115) Kafka brokers have a config called "offsets.topic.replication.factor" that specify the replication factor for the __consumer_offsets topic. The problem is that this config isn't being enforced upon auto topic creation. If an attempt to auto create the internal topic is made when there are fewer brokers than "offsets.topic.replication.factor", the topic ends up getting created anyway with the current number of live brokers. The current behavior is pretty surprising when you have clients or tooling running as the cluster is getting setup. Even if your cluster ends up being huge, you'll find out much later that __consumer_offsets was setup with no replication. The cluster not meeting the "offsets.topic.replication.factor" requirement on the internal topic is another way of saying the cluster isn't fully setup yet. The right behavior should be for "offsets.topic.replication.factor" to be enforced. Internal topic creation can happen in five paths: 1. By a broker upon GroupCoordinatorRequest. 2. By a broker from MetadataRequest querying the internal topic even if "auto.create.topics.enable" is false. 3. By a user when using AdminUtils. 4. By a user when running kafka-topics (which calls AdminUtils). 5. By a broker through AdminManager (which calls AdminUtils) handling CreateTopicsRequest. 6. By a user directly writing to the topic znode in zookeeper. Consequences of this patch: 1. will now fail topic creation of the internal topic with GROUP_COORDINATOR_NOT_AVAILABLE until the "offsets.topic.replication.factor" requirement is met. 2. will now fail topic creation of the internal topic and retain existing behavior of failing topic creation with INVALID_REPLICATION_FACTOR until the "offsets.topic.replication.factor" requirement is met. 3. will retain existing behavior. AdminUtils compares cluster size vs. replication factor comparison and throws an InvalidReplicationFactorException if the manually specified replication factor isn't met. If the replication factor is met, it creates the topic, ignoring the broker's offsets.topic.replication.factor config. 4. Same as 3. 5. Same as 3. CreateTopicsResponse including an internal topic will return INVALID_REPLICATION_FACTOR if the manually specified replication factor isn't met. 6. is unrelated, as the zk write will not receive any error from kafka.
onurkaraman
force-pushed
the
KAFKA-3959
branch
from
b2b4332
to
6d643af
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
onurkaraman
changed the title
|
Was about to check this in, but I didn't see any notes about validating the system tests still work. Kicked off https://jenkins.confluent.io/job/system-test-kafka-branch-builder-2/187/ to make sure they are still in good shape with just the minor template tweak. |
|
@ewencp looks like https://jenkins.confluent.io/job/system-test-kafka-branch-builder-2/187/ failed. I kicked off a new build with the same params as build 187 and it passed: https://jenkins.confluent.io/job/system-test-kafka-branch-builder-2/188/ |
|
Thanks @onurkaraman. LGTM, committing to trunk for inclusion in 0.10.3.0. |
|
@ewencp Thanks for the check-in. I just noticed that the checked in code has the upgrade notes under "Notable changes in 0.10.2.0" while I think we agreed to put it in 0.10.3.0. |
|
@onurkaraman Good catch, somehow missed this despite re-reviewing the changes. Filed #2484 and reopened the JIRA to adjust the docs. |
…offsets auto topic creation (KIP-115) Kafka brokers have a config called "offsets.topic.replication.factor" that specify the replication factor for the "__consumer_offsets" topic. The problem is that this config isn't being enforced. If an attempt to create the internal topic is made when there are fewer brokers than "offsets.topic.replication.factor", the topic ends up getting created anyway with the current number of live brokers. The current behavior is pretty surprising when you have clients or tooling running as the cluster is getting setup. Even if your cluster ends up being huge, you'll find out much later that __consumer_offsets was setup with no replication. The cluster not meeting the "offsets.topic.replication.factor" requirement on the internal topic is another way of saying the cluster isn't fully setup yet. The right behavior should be for "offsets.topic.replication.factor" to be enforced. Topic creation of the internal topic should fail with GROUP_COORDINATOR_NOT_AVAILABLE until the "offsets.topic.replication.factor" requirement is met. This closely resembles the behavior of regular topic creation when the requested replication factor exceeds the current size of the cluster, as the request fails with error INVALID_REPLICATION_FACTOR. Author: Onur Karaman <okaraman@linkedin.com> Reviewers: Jason Gustafson <jason@confluent.io>, Ismael Juma <ismael@juma.me.uk>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#2177 from onurkaraman/KAFKA-3959
Kafka brokers have a config called "offsets.topic.replication.factor" that specify the replication factor for the "__consumer_offsets" topic. The problem is that this config isn't being enforced. If an attempt to create the internal topic is made when there are fewer brokers than "offsets.topic.replication.factor", the topic ends up getting created anyway with the current number of live brokers. The current behavior is pretty surprising when you have clients or tooling running as the cluster is getting setup. Even if your cluster ends up being huge, you'll find out much later that __consumer_offsets was setup with no replication.
The cluster not meeting the "offsets.topic.replication.factor" requirement on the internal topic is another way of saying the cluster isn't fully setup yet.
The right behavior should be for "offsets.topic.replication.factor" to be enforced. Topic creation of the internal topic should fail with GROUP_COORDINATOR_NOT_AVAILABLE until the "offsets.topic.replication.factor" requirement is met. This closely resembles the behavior of regular topic creation when the requested replication factor exceeds the current size of the cluster, as the request fails with error INVALID_REPLICATION_FACTOR.