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-3267: Describe and Alter Configs Admin APIs (KIP-133) #3076

Closed

Conversation

@ijuma
Copy link
Contributor

commented May 16, 2017

No description provided.

@asfbot

This comment has been minimized.

Copy link

commented May 16, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 16, 2017

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

@ijuma ijuma force-pushed the ijuma:kafka-3267-describe-alter-configs-protocol branch 2 times, most recently May 17, 2017
@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@ijuma ijuma force-pushed the ijuma:kafka-3267-describe-alter-configs-protocol branch May 17, 2017
@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@ijuma ijuma force-pushed the ijuma:kafka-3267-describe-alter-configs-protocol branch 2 times, most recently May 17, 2017
@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@ijuma ijuma force-pushed the ijuma:kafka-3267-describe-alter-configs-protocol branch 2 times, most recently May 17, 2017
@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 17, 2017

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

Copy link
Contributor

left a comment

@ijuma : Thanks for the patch. Looks good overall. Left a few comments.

clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java Outdated
new Field("resources", new ArrayOf(DESCRIBE_CONFIGS_RESPONSE_ENTITY_V0)));

public static final Schema[] LIST_CONFIGS_REQUEST = {DESCRIBE_CONFIGS_REQUEST_V0};
public static final Schema[] LIST_CONFIGS_RESPONSE = {DESCRIBE_CONFIGS_RESPONSE_V0};

This comment has been minimized.

Copy link
@junrao

junrao May 18, 2017

Contributor

LIST_CONFIGS => DESCRIBE_CONFIGS to be consistent?

if (validateOnly)
AdminUtils.validateTopicConfig(zkUtils, topic, properties)
else
AdminUtils.changeTopicConfig(zkUtils, topic, properties)

This comment has been minimized.

Copy link
@junrao

junrao May 18, 2017

Contributor

With the new alter config api, it seems the config validations in TopicConfigHandler.processConfigChanges() can be done here so that we can indicate an error to the client. There may be other configs that we can validate such as min.isr = # replicas.

This comment has been minimized.

Copy link
@ijuma

ijuma May 18, 2017

Author Contributor

That's a good suggestion. I think we can do that in a subsequent PR, so filed KAFKA-5272.

throw new InvalidRequestException(s"Broker id must be an integer, but it is: ${resource.name}")
}
if (brokerId == config.brokerId)
createResponseConfig(config, isReadOnly = true, name => !config.originals.containsKey(name))

This comment has been minimized.

Copy link
@junrao

junrao May 18, 2017

Contributor

Does this cover those configs for customized listeners? It seems that config.values only include those originals specified in configDef which doesn't include the configures for customized listeners?

This comment has been minimized.

Copy link
@ijuma

ijuma May 18, 2017

Author Contributor

Great point. This requires a bit of care, so I filed KAFKA-5276. I think we can do that in a subsequent PR.

@@ -31,9 +31,10 @@ object AclCommand {

val Newline = scala.util.Properties.lineSeparator
val ResourceTypeToValidOperations = Map[ResourceType, Set[Operation]] (
Topic -> Set(Read, Write, Describe, All, Delete),
Broker -> Set(DescribeConfigs),

This comment has been minimized.

Copy link
@junrao

junrao May 18, 2017

Contributor

It seems that we need to support Broker resource in getResource() and the cli option?

clients/src/main/java/org/apache/kafka/clients/admin/AdminClient.java Outdated
@@ -194,4 +194,18 @@ public ApiVersionsResults apiVersions(Collection<Node> nodes) {
* @return The ApiVersionsResults.
*/
public abstract ApiVersionsResults apiVersions(Collection<Node> nodes, ApiVersionsOptions options);

public DescribeConfigsResults describeConfigs(Collection<ConfigResource> resources) {

This comment has been minimized.

Copy link
@junrao

junrao May 18, 2017

Contributor

Could we add a comment that describes the new methods?

This comment has been minimized.

Copy link
@ijuma

ijuma May 18, 2017

Author Contributor

Sure, I did it for the AdminClient methods. I intend to do a Javadoc pass over all AdminClient classes, filed KAFKA-5274 to track that.

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java Outdated
@@ -647,7 +667,7 @@ private long sendEligibleCalls(long now, Map<Node, List<Call>> callsToSend,
continue;
}
ClientRequest clientRequest = client.newClientRequest(node.idString(), requestBuilder, now, true);
log.trace("{}: sending {} to {}. correlationId={}", clientId, requestBuilder, node,
log.error("{}: sending {} to {}. correlationId={}", clientId, requestBuilder, node,

This comment has been minimized.

Copy link
@junrao

junrao May 18, 2017

Contributor

Should this be error logging?

This comment has been minimized.

Copy link
@ijuma

ijuma May 18, 2017

Author Contributor

No, it was a leftover from debugging.

core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala Outdated
ApiKeys.DELETE_TOPICS -> ((resp: requests.DeleteTopicsResponse) => resp.errors.asScala.find(_._1 == deleteTopic).get._2),
ApiKeys.OFFSET_FOR_LEADER_EPOCH -> ((resp: OffsetsForLeaderEpochResponse) => resp.responses.get(tp).error),
ApiKeys.DESCRIBE_CONFIGS -> { (resp: DescribeConfigsResponse) =>
if (resp.configs.get(tp) == null)

This comment has been minimized.

Copy link
@junrao

junrao May 18, 2017

Contributor

Hmm, is this code intended to be here? resp.configs is not keyed on tp.

This comment has been minimized.

Copy link
@ijuma

ijuma May 18, 2017

Author Contributor

Nope, my bad.

core/src/test/scala/integration/kafka/api/KafkaAdminClientIntegrationTest.scala Outdated
assertEquals(servers(1).config.brokerId.toString, configs.get(brokerResource1).get(KafkaConfig.BrokerIdProp).value)
val controllerSocketTimeoutMs = configs.get(brokerResource1).get(KafkaConfig.ControllerSocketTimeoutMsProp)
assertEquals(servers(1).config.controllerSocketTimeoutMs.toString, controllerSocketTimeoutMs.value)
assertEquals(KafkaConfig.ControllerSocketTimeoutMsProp, controllerSocketTimeoutMs.name)

This comment has been minimized.

Copy link
@junrao

junrao May 18, 2017

Contributor

Perhaps pick a config overridden in generateConfigs() instead in TestUtils?

@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@ijuma ijuma force-pushed the ijuma:kafka-3267-describe-alter-configs-protocol branch May 18, 2017
@junrao

This comment has been minimized.

Copy link
Contributor

commented May 18, 2017

@ijuma : Thanks for the updated patch. LGTM. I will let you merge it after the test passes.

@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@ijuma ijuma force-pushed the ijuma:kafka-3267-describe-alter-configs-protocol branch May 18, 2017
@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@ijuma ijuma force-pushed the ijuma:kafka-3267-describe-alter-configs-protocol branch to 1966922 May 18, 2017
@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@asfbot

This comment has been minimized.

Copy link

commented May 18, 2017

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

@asfgit asfgit closed this in 972b754 May 18, 2017
@ijuma ijuma deleted the ijuma:kafka-3267-describe-alter-configs-protocol branch Sep 5, 2017
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.