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-7002: Add a config property for DLQ topic's replication factor (KIP-298) #5145

Closed
wants to merge 2 commits into from

Conversation

@wicknicks
Copy link
Contributor

commented Jun 6, 2018

Currently, the replication factor is hardcoded to a value of 3. This means that we cannot use a DLQ in any cluster setup with less than three brokers. It is better to have the user specify this value if the default value does meet the requirements.

Testing: A unit test is added.

Signed-off-by: Arjun Satish arjun@confluent.io

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)
…(KIP-298)

Signed-off-by: Arjun Satish <arjun@confluent.io>
@wicknicks wicknicks force-pushed the wicknicks:KAFKA-7002 branch to 11d2ac0 Jun 6, 2018
@wicknicks wicknicks changed the title KAFKA-7002: Add a config property for DLQ topic's replication factor KAFKA-7002: Add a config property for DLQ topic's replication factor (KIP-298) Jun 6, 2018
@wicknicks

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2018

@mageshn @rayokota @rhauch any thoughts on this PR?

@rhauch
rhauch approved these changes Jun 6, 2018
Copy link
Contributor

left a comment

One very minor but optional suggestion. Otherwise LGTM. Thanks!

@@ -52,10 +52,16 @@
public static final String DLQ_TOPIC_DEFAULT = "";
private static final String DLQ_TOPIC_DISPLAY = "Dead Letter Queue Topic Name";

public static final String DLQ_TOPIC_REPLICATION_FACTOR_CONFIG = DLQ_PREFIX + "topic.replication.factor";
private static final String DLQ_TOPIC_REPLICATION_FACTOR_CONFIG_DOC = "Replication factor used when creating the dead letter queue topic";

This comment has been minimized.

Copy link
@rhauch

rhauch Jun 6, 2018

Contributor

Might want to add that this is not used if the topic already exists.

Signed-off-by: Arjun Satish <arjun@confluent.io>
@wicknicks

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2018

@ewencp could you please take a look at this PR, and merge it if things look good? Thanks!

@ewencp
ewencp approved these changes Jun 7, 2018
Copy link
Contributor

left a comment

This looks fine, but in our example configs we also set other replication factors to values that will work for a single, local instance even in distributed mode. Should we include that as well? It's more critical for the ones that are in that file because they are required to even start up, but seems like if we're sticking to example values that work on a single node "cluster", we'd want to make sure all features would work there.

@wicknicks

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2018

@ewencp happy to update the example. But as of now, DLQ is disabled by default. if we add this config in the example files, we will need to enable error handling, enable DLQ and then set the replication factor. I think it is ok to exclude it from the current example for the single node setup. Thoughts?

@ewencp

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

Hmm, fair point, guess it kinda sucks to have to set the parent one as well. Let me merge this for now, and we can follow up on whether having this default but not overriding it in the example configs might cause problems.

Only semi-related, but we might also want to think about adding some production-grade example configs at some point.

Merging as is to trunk and 2.0, and we can follow up separately if we need to.

ewencp added a commit that referenced this pull request Jun 7, 2018
…(KIP-298)

Currently, the replication factor is hardcoded to a value of 3. This means that we cannot use a DLQ in any cluster setup with less than three brokers. It is better to have the user specify this value if the default value does meet the requirements.

Testing: A unit test is added.

Signed-off-by: Arjun Satish <arjunconfluent.io>

Author: Arjun Satish <arjun@confluent.io>

Reviewers: Randall Hauch <rhauch@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io>

Closes #5145 from wicknicks/KAFKA-7002

(cherry picked from commit 22612be)
Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
@ewencp ewencp closed this in 22612be Jun 7, 2018
ying-zheng added a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
…(KIP-298)

Currently, the replication factor is hardcoded to a value of 3. This means that we cannot use a DLQ in any cluster setup with less than three brokers. It is better to have the user specify this value if the default value does meet the requirements.

Testing: A unit test is added.

Signed-off-by: Arjun Satish <arjunconfluent.io>

Author: Arjun Satish <arjun@confluent.io>

Reviewers: Randall Hauch <rhauch@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io>

Closes apache#5145 from wicknicks/KAFKA-7002
nimosunbit added a commit to sunbit-dev/kafka that referenced this pull request Nov 6, 2018
…(KIP-298)

Currently, the replication factor is hardcoded to a value of 3. This means that we cannot use a DLQ in any cluster setup with less than three brokers. It is better to have the user specify this value if the default value does meet the requirements.

Testing: A unit test is added.

Signed-off-by: Arjun Satish <arjunconfluent.io>

Author: Arjun Satish <arjun@confluent.io>

Reviewers: Randall Hauch <rhauch@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io>

Closes apache#5145 from wicknicks/KAFKA-7002
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.