KAFKA-7048 NPE when creating connector #5202

chia7712 · 2018-06-12T17:27:31Z

Committer Checklist (excluded from commit message)

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

rajinisivaram · 2018-06-15T12:19:29Z

@rayokota Can you review this PR?

rayokota · 2018-06-15T15:18:57Z

...t/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/ClusterConfigState.java

@@ -112,12 +112,13 @@ public boolean contains(String connector) {
     * {@link org.apache.kafka.common.config.ConfigTransformer} by having all variable
     * references replaced with the current values from external instances of
     * {@link ConfigProvider}, and may include secrets.
+     * NOTED: WorkerConfigTransformer#transform won't be called if the configuration of task doesn't exist.


Perhaps replace this NOTE with If the configuration is not null, it will have been transformed...

rayokota · 2018-06-15T15:19:04Z

...t/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/ClusterConfigState.java

@@ -137,12 +138,13 @@ public TargetState targetState(String connector) {
     * {@link org.apache.kafka.common.config.ConfigTransformer} by having all variable
     * references replaced with the current values from external instances of
     * {@link ConfigProvider}, and may include secrets.
+     * NOTED: WorkerConfigTransformer#transform won't be called if the configuration of task doesn't exist.


Perhaps replace this NOTE with If the configuration is not null, it will have been transformed...

rayokota · 2018-06-15T17:47:01Z

@rajinisivaram , just some small changes to JavaDoc, otherwise LGTM

chia7712 · 2018-06-15T19:27:55Z

Thank @rayokota for the reviews. Have addressed your comment.

rayokota

Small nits (because I wasn't that clear), but otherwise LGTM

rayokota · 2018-06-15T19:56:50Z

...t/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/ClusterConfigState.java

@@ -112,12 +112,13 @@ public boolean contains(String connector) {
     * {@link org.apache.kafka.common.config.ConfigTransformer} by having all variable
     * references replaced with the current values from external instances of
     * {@link ConfigProvider}, and may include secrets.
+     * NOTED: If the configuration is not null, it will have been transformed.


Nit: Sorry, I wasn't that a clear. I meant to remove this NOTE and start the JavaDoc with:

/** * Get the configuration for a connector. If the configuration is not null, * it will have been transformed by

rayokota · 2018-06-15T19:57:33Z

...t/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/ClusterConfigState.java

@@ -137,12 +138,13 @@ public TargetState targetState(String connector) {
     * {@link org.apache.kafka.common.config.ConfigTransformer} by having all variable
     * references replaced with the current values from external instances of
     * {@link ConfigProvider}, and may include secrets.
+     * NOTED: If the configuration is not null, it will have been transformed.


Nit: Sorry, I wasn't that a clear. I meant to remove this NOTE and start the JavaDoc with:

/** * Get the configuration for a task. If the configuration is not null, * it will have been transformed by

My bad. :(
Will correct the docs in next commit.

kkonstantine · 2018-06-15T21:56:14Z

...t/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/ClusterConfigState.java

     * @param connector name of the connector
     * @return a map containing configuration parameters
     */
    public Map<String, String> connectorConfig(String connector) {
        Map<String, String> configs = connectorConfigs.get(connector);
-        if (configTransformer != null) {
+        if (configs != null && configTransformer != null) {


I wonder if a better location for null checking configs is inside transform. To me, it seems it is, because this way the check is in one place and we are protected against future uses of tranform with a null argument.

Also, in contrast, seems that configTransformer can not be null.

Also, in contrast, seems that configTransformer can not be null.

Not really. In DistributedHerder's constructor, the first configState is ClusterConfigState.EMPTY. And the ClusterConfigState.EMPTY has a null configTransformer.

We can introduce a interface of WorkerConfigTransformer and then pass a do-nothing impl to create an empty ClusterConfigState. Consequently, we can make the configTransformer in ClusterConfigState non-nullable.

I wonder if a better location for null checking configs is inside transform. To me, it seems it is, because this way the check is in one place and we are protected against future uses of tranform with a null argument.

It makes sense to me. Will address this comment in next commit

…to WorkerConfigTransformer

Reviewers: Robert Yokota <rayokota@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

guozhangwang · 2018-06-17T23:50:10Z

Cherry-picked to 2.0

Reviewers: Robert Yokota <rayokota@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

chia7712 force-pushed the KAFKA-7048 branch from aa225ba to 8fca72e Compare June 13, 2018 10:38

rayokota suggested changes Jun 15, 2018

View reviewed changes

rayokota approved these changes Jun 15, 2018

View reviewed changes

kkonstantine reviewed Jun 15, 2018

View reviewed changes

chia7712 added 3 commits June 17, 2018 14:04

KAFKA-7048 NPE when creating connector

99d9043

KAFKA-7048 (addendum) address Robert's comment - revise the javadocs

7cb3b83

KAFKA-7048 (addendum) address Robert's comment - move the null check …

2bfb46e

…to WorkerConfigTransformer

chia7712 force-pushed the KAFKA-7048 branch from 718703a to 2bfb46e Compare June 17, 2018 06:04

guozhangwang merged commit 6810617 into apache:trunk Jun 17, 2018

guozhangwang pushed a commit that referenced this pull request Jun 17, 2018

KAFKA-7048 NPE when creating connector (#5202)

d1ebab5

Reviewers: Robert Yokota <rayokota@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018

KAFKA-7048 NPE when creating connector (apache#5202)

c61568d

Reviewers: Robert Yokota <rayokota@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

kkonstantine added the connect label Oct 16, 2020

chia7712 deleted the KAFKA-7048 branch March 25, 2024 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-7048 NPE when creating connector #5202

KAFKA-7048 NPE when creating connector #5202

chia7712 commented Jun 12, 2018

rajinisivaram commented Jun 15, 2018

rayokota Jun 15, 2018

rayokota Jun 15, 2018

rayokota commented Jun 15, 2018

chia7712 commented Jun 15, 2018

rayokota left a comment

rayokota Jun 15, 2018

rayokota Jun 15, 2018

chia7712 Jun 16, 2018

kkonstantine Jun 15, 2018

chia7712 Jun 16, 2018 •

edited

Loading

chia7712 Jun 17, 2018

guozhangwang commented Jun 17, 2018

KAFKA-7048 NPE when creating connector #5202

KAFKA-7048 NPE when creating connector #5202

Conversation

chia7712 commented Jun 12, 2018

Committer Checklist (excluded from commit message)

rajinisivaram commented Jun 15, 2018

rayokota Jun 15, 2018

Choose a reason for hiding this comment

rayokota Jun 15, 2018

Choose a reason for hiding this comment

rayokota commented Jun 15, 2018

chia7712 commented Jun 15, 2018

rayokota left a comment

Choose a reason for hiding this comment

rayokota Jun 15, 2018

Choose a reason for hiding this comment

rayokota Jun 15, 2018

Choose a reason for hiding this comment

chia7712 Jun 16, 2018

Choose a reason for hiding this comment

kkonstantine Jun 15, 2018

Choose a reason for hiding this comment

chia7712 Jun 16, 2018 • edited Loading

Choose a reason for hiding this comment

chia7712 Jun 17, 2018

Choose a reason for hiding this comment

guozhangwang commented Jun 17, 2018

chia7712 Jun 16, 2018 •

edited

Loading