KAFKA-5274: AdminClient Javadoc improvements #3316

ijuma · 2017-06-13T14:30:19Z

Publish Javadoc for common.annotation package, which contains
InterfaceStability.

Finally, mark AdminClient classes with Evolving instead of Unstable.

asfbot · 2017-06-13T14:37:47Z

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

asfbot · 2017-06-13T14:39:23Z

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

asfbot · 2017-06-13T16:44:27Z

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

asfbot · 2017-06-13T16:46:38Z

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

cmccabe · 2017-06-13T23:25:33Z

clients/src/main/java/org/apache/kafka/clients/admin/AlterConfigsResult.java

    public Map<ConfigResource, KafkaFuture<Void>> results() {
        return futures;
    }

+    /**
+     * Return a future which succeeds only if all the alter configs succeed.


Perhaps add "operation"? So "only if all the alter configs operations succeed." ?

cmccabe · 2017-06-13T23:28:25Z

clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java

+    /**
+     * Return whether the config value is sensitive. The value is always set to null by the broker if the config value
+     * is sensitive.
+     */
    public boolean isSensitive() {


Is it more kafka-like to name this 'sensitive' instead of isSensitive?

Kafka drops getters, but booleans are a little difference in that they can be pretty unclear without a prefix. I noticed this and a few other inconsistencies, but wanted to keep this PR purely as Javadoc changes.

cmccabe · 2017-06-13T23:28:39Z

clients/src/main/java/org/apache/kafka/clients/admin/ConfigEntry.java

    public boolean isSensitive() {
        return isSensitive;
    }

+    /**
+     * Return whether the config is read-only and cannot be updated.
+     */
    public boolean isReadOnly() {


readOnly instead of isReadOnly?

Same as my other comment about limiting this PR to Javadoc changes.

cmccabe · 2017-06-13T23:32:23Z

clients/src/main/java/org/apache/kafka/clients/admin/TopicDescription.java

    public boolean internal() {
        return internal;
    }

+    /**
+     * A map from partition id to its leadership and replica information.


How about "* A map from partition id to the leadership and replica information for that partition"

cmccabe · 2017-06-13T23:33:21Z

clients/src/main/java/org/apache/kafka/common/acl/AccessControlEntry.java

    public AccessControlEntry(String principal, String host, AclOperation operation, AclPermissionType permissionType) {
        Objects.requireNonNull(principal);
        Objects.requireNonNull(host);
        Objects.requireNonNull(operation);
-        assert operation != AclOperation.ANY;
+        assert(operation != AclOperation.ANY);
        Objects.requireNonNull(permissionType);
        assert permissionType != AclPermissionType.ANY;


Should change this one as well if you are going to change the assert above to have parentheses

That was accidental, I have fixed it locally, but didn't push it yet.

cmccabe

Looks good overall. Does it make sense to do the ExtendedDeserializer, ExtendedSerializer, Serializer in this patch, or should we have another one fo that, since they're not really related to adminclient...? [edit: oh, I see you have that in the PR description... never mind]

ijuma · 2017-06-13T23:37:51Z

@cmccabe, I can do a separate PR for the Serializer changes, if you prefer.

cmccabe · 2017-06-14T01:04:13Z

The adminclient parts LGTM.

I do have a preference to doing the serializer changes in a separate PR, although I don't feel strongly about it.

ijuma · 2017-06-14T01:30:16Z

Moved serializer javadoc changes to a separate PR as requested: #3330

ijuma · 2017-06-14T01:31:03Z

@gwenshap or @rajinisivaram, maybe one of you can review and merge this javadoc PR. @cmccabe has reviewed it.

asfbot · 2017-06-14T01:53:43Z

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

asfbot · 2017-06-14T01:56:23Z

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

asfbot · 2017-06-14T02:49:25Z

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

asfbot · 2017-06-14T03:09:08Z

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

It includes InterfaceStability annotation.

asfbot · 2017-06-14T12:52:25Z

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

asfbot · 2017-06-14T13:50:09Z

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

ijuma · 2017-06-14T14:36:54Z

Test failure is unrelated.

asfbot · 2017-06-14T14:42:08Z

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

asfbot · 2017-06-14T14:42:19Z

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

asfbot · 2017-06-14T15:48:43Z

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

gwenshap · 2017-06-14T15:57:09Z

LGTM. Really nice work, @ijuma.

Publish Javadoc for common.annotation package, which contains InterfaceStability. Finally, mark AdminClient classes with `Evolving` instead of `Unstable`. Author: Ismael Juma <ismael@juma.me.uk> Reviewers: Colin Mccabe, Gwen Shapira Closes #3316 from ijuma/kafka-5274-admin-client-javadoc (cherry picked from commit b20d933) Signed-off-by: Gwen Shapira <cshapi@gmail.com>

asfbot · 2017-06-14T16:14:38Z

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

ijuma force-pushed the kafka-5274-admin-client-javadoc branch from 38ba7be to 5f1e1c9 Compare June 13, 2017 16:27

cmccabe reviewed Jun 13, 2017

View reviewed changes

ijuma changed the title ~~[WIP] KAFKA-5274: AdminClient Javadoc improvements~~ KAFKA-5274: AdminClient Javadoc improvements Jun 14, 2017

ijuma force-pushed the kafka-5274-admin-client-javadoc branch from 9ad4edb to e3df61c Compare June 14, 2017 01:29

ijuma added 8 commits June 14, 2017 13:00

Publish javadoc for common.annotation package

515037d

It includes InterfaceStability annotation.

Clarify InterfaceStability

51264de

Various javadoc improvements to API classes in clients.admin

c5d77c5

Revert assert change

57d83d0

Add javadoc to some classes in common

d5a1bb4

More javadoc for common classes

a5bb109

Address review feedback

baffc69

Document TopicPartitionInfo

ac06459

ijuma force-pushed the kafka-5274-admin-client-javadoc branch from e3df61c to ac06459 Compare June 14, 2017 12:00

Document broker requirement for AdminClient methods

72a0010

Add InterfaceStability to more classes

1a65c10

ijuma force-pushed the kafka-5274-admin-client-javadoc branch from ebc623b to 1a65c10 Compare June 14, 2017 14:52

ijuma mentioned this pull request Jun 14, 2017

KAFKA-5275: AdminClient API consistency #3339

Closed

asfgit closed this in b20d933 Jun 14, 2017

ijuma deleted the kafka-5274-admin-client-javadoc branch June 18, 2017 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-5274: AdminClient Javadoc improvements #3316

KAFKA-5274: AdminClient Javadoc improvements #3316

ijuma commented Jun 13, 2017 •

edited

Loading

asfbot commented Jun 13, 2017

asfbot commented Jun 13, 2017

asfbot commented Jun 13, 2017

asfbot commented Jun 13, 2017

cmccabe Jun 13, 2017

cmccabe Jun 13, 2017

ijuma Jun 13, 2017

cmccabe Jun 13, 2017

cmccabe Jun 13, 2017

ijuma Jun 13, 2017

cmccabe Jun 14, 2017

cmccabe Jun 13, 2017

cmccabe Jun 13, 2017

ijuma Jun 13, 2017

cmccabe left a comment •

edited

Loading

ijuma commented Jun 13, 2017

cmccabe commented Jun 14, 2017

ijuma commented Jun 14, 2017

ijuma commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

ijuma commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

gwenshap commented Jun 14, 2017

asfbot commented Jun 14, 2017

KAFKA-5274: AdminClient Javadoc improvements #3316

KAFKA-5274: AdminClient Javadoc improvements #3316

Conversation

ijuma commented Jun 13, 2017 • edited Loading

asfbot commented Jun 13, 2017

asfbot commented Jun 13, 2017

asfbot commented Jun 13, 2017

asfbot commented Jun 13, 2017

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

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

cmccabe left a comment • edited Loading

Choose a reason for hiding this comment

ijuma commented Jun 13, 2017

cmccabe commented Jun 14, 2017

ijuma commented Jun 14, 2017

ijuma commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

ijuma commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

gwenshap commented Jun 14, 2017

asfbot commented Jun 14, 2017

ijuma commented Jun 13, 2017 •

edited

Loading

cmccabe left a comment •

edited

Loading