You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by olegz <gi...@git.apache.org> on 2016/07/29 13:07:52 UTC
[GitHub] nifi pull request #741: NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
GitHub user olegz opened a pull request:
https://github.com/apache/nifi/pull/741
NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
- Fixed KafkaConsumer's connection block when broker is not available
- Fixed Serializer/Deserializer configs in both Consume/Publish Kafka
- Added sensitive properties for SSL ket/trust stores
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/olegz/nifi NIFI-2322
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/741.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #741
----
commit bd9172473293c6be4d0287b62f8c91a4b068bd8b
Author: Oleg Zhurakousky <ol...@suitcase.io>
Date: 2016-07-29T13:06:47Z
NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
- Fixed KafkaConsumer's connection block when broker is not available
- Fixed Serializer/Deserializer configs in both Consume/Publish Kafka
- Added sensitive properties for SSL ket/trust stores
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #741: NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on the issue:
https://github.com/apache/nifi/pull/741
@olegz I'm seeing failed tests
```
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running org.apache.nifi.processors.kafka.pubsub.AbstractKafkaProcessorLifecycleTest
Tests run: 5, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.471 sec - in org.apache.nifi.processors.kafka.pubsub.AbstractKafkaProcessorLifecycleTest
Running org.apache.nifi.processors.kafka.pubsub.ConsumeKafkaTest
Tests run: 3, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.062 sec <<< FAILURE! - in org.apache.nifi.processors.kafka.pubsub.ConsumeKafkaTest
validateGetAllMessagesWithProvidedDemarcator(org.apache.nifi.processors.kafka.pubsub.ConsumeKafkaTest) Time elapsed: 0.048 sec <<< FAILURE!
java.lang.AssertionError: expected:<1> but was:<0>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:834)
at org.junit.Assert.assertEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:631)
at org.apache.nifi.processors.kafka.pubsub.ConsumeKafkaTest.validateGetAllMessagesWithProvidedDemarcator(ConsumeKafkaTest.java:140)
validateGetAllMessages(org.apache.nifi.processors.kafka.pubsub.ConsumeKafkaTest) Time elapsed: 0.011 sec <<< FAILURE!
java.lang.AssertionError: expected:<6> but was:<0>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:834)
at org.junit.Assert.assertEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:631)
at org.apache.nifi.processors.kafka.pubsub.ConsumeKafkaTest.validateGetAllMessages(ConsumeKafkaTest.java:102)
Running org.apache.nifi.processors.kafka.pubsub.KafkaPublisherTest
Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.382 sec - in org.apache.nifi.processors.kafka.pubsub.KafkaPublisherTest
Running org.apache.nifi.processors.kafka.pubsub.PublishingContextTest
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.008 sec - in org.apache.nifi.processors.kafka.pubsub.PublishingContextTest
Running org.apache.nifi.processors.kafka.pubsub.PublishKafkaTest
Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.065 sec - in org.apache.nifi.processors.kafka.pubsub.PublishKafkaTest
Results :
Failed tests:
ConsumeKafkaTest.validateGetAllMessages:102 expected:<6> but was:<0>
ConsumeKafkaTest.validateGetAllMessagesWithProvidedDemarcator:140 expected:<1> but was:<0>
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #741: NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/741#discussion_r73333456
--- Diff: nifi-nar-bundles/nifi-kafka-bundle/nifi-kafka-pubsub-processors/src/main/java/org/apache/nifi/processors/kafka/pubsub/AbstractKafkaProcessor.java ---
@@ -141,6 +166,10 @@
SHARED_DESCRIPTORS.add(CLIENT_ID);
SHARED_DESCRIPTORS.add(SECURITY_PROTOCOL);
SHARED_DESCRIPTORS.add(KERBEROS_PRINCIPLE);
+ SHARED_DESCRIPTORS.add(SSL_KEY_PASSWORD);
--- End diff --
This feels very odd to me to provide properties for the passwords but not the keystore and truststore. I do realize that the keystore and truststore can be specified via user-defined properties, but it adds to the complexity of configuring this, as the user has to lookup the appropriate property names. This is a usability miss.
Why do we not simply expose a property for an SSLContextService? We could then pull the appropriate values from the SSLContextService via getKeyStorePassword(), getKeyStoreFile(), etc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #741: NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/741#discussion_r73512333
--- Diff: nifi-nar-bundles/nifi-kafka-bundle/nifi-kafka-pubsub-processors/src/main/java/org/apache/nifi/processors/kafka/pubsub/AbstractKafkaProcessor.java ---
@@ -141,6 +166,10 @@
SHARED_DESCRIPTORS.add(CLIENT_ID);
SHARED_DESCRIPTORS.add(SECURITY_PROTOCOL);
SHARED_DESCRIPTORS.add(KERBEROS_PRINCIPLE);
+ SHARED_DESCRIPTORS.add(SSL_KEY_PASSWORD);
--- End diff --
@olegz I do agree that it would be nice to allow users to explicitly mark dynamic properties as sensitive. However, even with that capability, providing the option of using an SSLContextService is still preferable, I believe, as it is typically much easier to configure, since you can configure the service once and then use anywhere. I have attached a patch to NIFI-2423 that incorporates the SSL Context Service. Can you please review & if good either merge into your PR or give it a +1 on the JIRA? Then I should be able to merge in this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #741: NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the issue:
https://github.com/apache/nifi/pull/741
ouch, the code that provided the fix for connection broke the test. Argh!!!! Give me a min
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #741: NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
Posted by olegz <gi...@git.apache.org>.
Github user olegz closed the pull request at:
https://github.com/apache/nifi/pull/741
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #741: NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the issue:
https://github.com/apache/nifi/pull/741
@brosander done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #741: NIFI-2322, NIFI-2423, NIFI-2412 Kafka improvements
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/741#discussion_r73352158
--- Diff: nifi-nar-bundles/nifi-kafka-bundle/nifi-kafka-pubsub-processors/src/main/java/org/apache/nifi/processors/kafka/pubsub/AbstractKafkaProcessor.java ---
@@ -141,6 +166,10 @@
SHARED_DESCRIPTORS.add(CLIENT_ID);
SHARED_DESCRIPTORS.add(SECURITY_PROTOCOL);
SHARED_DESCRIPTORS.add(KERBEROS_PRINCIPLE);
+ SHARED_DESCRIPTORS.add(SSL_KEY_PASSWORD);
--- End diff --
If it is possible without any hackery I am all for it. The main issue for these once is that they were always available as dynamic properties along with all other Kafka properties but since they are 'passwords' and we do not support "sensitive" attribute for dynamic properties they were added as first class properties.
While it may work with SSLContextService for this case, the correct way of fixing something like this would be to provide support for sensitivity for dynamic properties
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---