Skip to content

KAFKA-10268: dynamic config like "--delete-config log.retention.ms" doesn't work#9051

Merged
huxihx merged 2 commits intoapache:trunkfrom
huxihx:KAFKA-10268
Jul 24, 2020
Merged

KAFKA-10268: dynamic config like "--delete-config log.retention.ms" doesn't work#9051
huxihx merged 2 commits intoapache:trunkfrom
huxihx:KAFKA-10268

Conversation

@huxihx
Copy link
Copy Markdown
Contributor

@huxihx huxihx commented Jul 22, 2020

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

Currently, ConfigCommand's --delete-config API does not restore the config to default value, no matter at broker-level or broker-default level. Besides, Admin.incrementalAlterConfigs API also runs into this problem. This patch fixes it by removing the corresponding config from the newConfig properties when reconfiguring dynamic broker config.

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

@huxi-2b
KAFKA-10268: dynamic config like "--delete-config log.retention.ms" d…
88226a4 
…oesn't work

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

Currently, ConfigCommand --delete-config API does not restore the config to default value, no matter at broker-level or broker-default level. Besides, Admin.incrementalAlterConfigs API also runs into this problem. This patch fixes it by removing the corresponding config from the newConfig properties when reconfiguring dynamic broker config.
@huxihx huxihx requested a review from rajinisivaram July 22, 2020 02:42
@huxihx
Copy link
Copy Markdown
Contributor Author

huxihx commented Jul 22, 2020

@rajinisivaram Please review this patch. Thanks.

dajac
dajac reviewed Jul 22, 2020
View reviewed changes
Comment thread core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala Outdated
val newProps = new Properties
newProps.put(KafkaConfig.LogRetentionTimeMillisProp, "100000")
newProps.put(KafkaConfig.LogFlushIntervalMsProp, "10000")
TestUtils.incrementalAlterConfigs(servers, adminClients.head, newProps, perBrokerConfig = false).all.get
Copy link
Copy Markdown
Member

@dajac dajac Jul 22, 2020

Choose a reason for hiding this comment

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

Shouldn't we also waitForConfigOnServer after the first alteration of the config?

Copy link
Copy Markdown
Contributor Author

@huxihx huxihx Jul 22, 2020

Choose a reason for hiding this comment

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

Thanks for the comments. We could add but not necessary since we care about the fact that the value for broker-default level should be overwritten by that for broker-level. Does it make sense?

Copy link
Copy Markdown
Member

@dajac dajac Jul 23, 2020

Choose a reason for hiding this comment

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

That makes sense, thanks.

rajinisivaram
rajinisivaram reviewed Jul 22, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

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

@huxihx Thanks for the PR, looks good. Left one minor comment about the test.

Comment thread core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala Outdated
}

@Test
def testDefaultValueRestoredAfterDeleteDynamicConfig(): Unit = {
Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram Jul 22, 2020

Choose a reason for hiding this comment

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

Can we move this into DynamicBrokerReconfigurationTest.testDefaultTopicConfig() and also verify that configs of existing topics have been updated? Or add a topic in this test to verify.

@rajinisivaram
Copy link
Copy Markdown
Contributor

rajinisivaram commented Jul 22, 2020

ok to test

@huxihx
Copy link
Copy Markdown
Contributor Author

huxihx commented Jul 23, 2020

retest this please

rajinisivaram
rajinisivaram approved these changes Jul 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

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

@huxihx Thanks for the update, LGTM

@huxihx
Copy link
Copy Markdown
Contributor Author

huxihx commented Jul 24, 2020

@rajinisivaram Thanks for the review, merging to trunk

@huxihx huxihx merged commit 1c439da into apache:trunk Jul 24, 2020
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jul 24, 2020

@huxihx Note that we usually include the reviewer(s) in the commit message. For example:

0d5c967

ijuma pushed a commit that referenced this pull request Jul 24, 2020
@huxi-2b @ijuma
KAFKA-10268: dynamic config like "--delete-config log.retention.ms" d…
67df232 
…oesn't work (#9051)

* KAFKA-10268: dynamic config like "--delete-config log.retention.ms" doesn't work

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

Currently, ConfigCommand --delete-config API does not restore the config to default value, no matter at broker-level or broker-default level. Besides, Admin.incrementalAlterConfigs API also runs into this problem. This patch fixes it by removing the corresponding config from the newConfig properties when reconfiguring dynamic broker config.
@rhauch
Copy link
Copy Markdown
Contributor

rhauch commented Jul 28, 2020

This was also cherry-picked to 2.6, but that branch has been frozen while we try to release AK 2.6.0. However, given that this is low-risk, I'll leave it on 2.6 and update KAFKA-10268 instead.

@huxihx, next time please refrain from cherry-picking to frozen branches. Thanks!

ijuma added a commit to ijuma/kafka that referenced this pull request Nov 17, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into skip-to-struc…
c430055 
…t-for-generated-requests

* apache-github/trunk: (148 commits)
  MINOR: remove NewTopic#NO_PARTITIONS and NewTopic#NO_REPLICATION_FACTOR as they are duplicate to CreateTopicsRequest#NO_NUM_PARTITIONS and CreateTopicsRequest#NO_REPLICATION_FACTOR (apache#9077)
  MINOR: Remove staticmethod tag to be able to use logger of instance (apache#9086)
  MINOR: Adjust 'release.py' script to use shell when using gradlewAll and PGP signing, which were required to build the 2.6.0 RCs (apache#9045)
  MINOR: Update dependencies for Kafka 2.7 (part 1) (apache#9082)
  MINOR: INFO log4j when request re-join (apache#9068)
  MINOR: Recommend Java 11 (apache#9080)
  KAFKA-10306: GlobalThread should fail on InvalidOffsetException (apache#9075)
  KAFKA-10158: Fix flaky testDescribeUnderReplicatedPartitionsWhenReassignmentIsInProgress (apache#9022)
  MINOR: code cleanup for `VOut` inconsistent naming (apache#8907)
  KAFKA-10246 : AbstractProcessorContext topic() throws NPE (apache#9034)
  KAFKA-10305: Print usage when parsing fails for ConsumerPerformance (apache#9071)
  MINOR: removed incorrect deprecation annotations (apache#9061)
  MINOR: speed up release script (apache#9070)
  MINOR: add task ':streams:testAll' (apache#9073)
  KAFKA-10301: Do not clear Partition#remoteReplicasMap during partition assignment updates (apache#9065)
  KAFKA-10268: dynamic config like "--delete-config log.retention.ms" doesn't work (apache#9051)
  KAFKA-10300 fix flaky core/group_mode_transactions_test.py (apache#9059)
  MINOR: Publish metrics package in the javadoc (apache#9036)
  KAFKA-8264: decrease the record size for flaky test
  KAFKA-5876: Add new exception types for Interactive Queries (apache#8200)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dajac dajac dajac left review comments

@rajinisivaram rajinisivaram rajinisivaram approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@huxihx @rajinisivaram @ijuma @rhauch @dajac @huxi-2b