You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by ijokarumawak <gi...@git.apache.org> on 2018/06/01 08:28:45 UTC
[GitHub] nifi pull request #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Exp...
GitHub user ijokarumawak opened a pull request:
https://github.com/apache/nifi/pull/2750
NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Couchbase integration
This PR includes two commits for NIFI-5054 and NIFI-5257.
NIFI-5054 can be merged separately, but NIFI-5054 depends on the dependency version bump in NIFI-5054.
These enhancements are all Couchbase Server related, and should be easy to review together.
---
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [x] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [x] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ijokarumawak/nifi nifi-5054
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2750.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 #2750
----
commit 4990269646b9dce529cb2c9adb4d70e7bba1beb0
Author: Koji Kawamura <ij...@...>
Date: 2018-06-01T07:55:26Z
NIFI-5054: Add Couchbase user authentication
commit b977450945f014aa9da23f086a53c2e0ac3e8b79
Author: Koji Kawamura <ij...@...>
Date: 2018-06-01T07:57:43Z
NIFI-5257: Expand Couchbase Server integration
- Added CouchbaseMapCacheClient.
- Added CouchbaseKeyValueLookupService.
- Added CouchbaseRecordLookupService.
- Added 'Put Value to Attribute' to GetCouchbaseKey.
- Fixed Get/PutCouchbaseKey relationship descriptions.
----
---
[GitHub] nifi issue #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Cou...
Posted by mgroves <gi...@git.apache.org>.
Github user mgroves commented on the issue:
https://github.com/apache/nifi/pull/2750
Hi @ijokarumawak I tried building this today (on Windows). I'm not much of a Java developer, so this could easily be a noob mistake. It looks like there was an error here:
```
> [ERROR] Errors:
> [ERROR] TestMinimalLockingWriteAheadLog.testRecoverFileThatHasTrailingNULBytesAndTruncation:503 ╗ OverlappingFileLock
> [INFO]
> [ERROR] Tests run: 30, Failures: 0, Errors: 1, Skipped: 3
>
```
Which leads to a build failure at the end:
```
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD FAILURE
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 01:38 min
> [INFO] Finished at: 2018-06-04T11:44:56-07:00
> [INFO] Final Memory: 134M/920M
> [INFO] ------------------------------------------------------------------------
> [ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.20.1:test (default-test) on project nifi-write-ahead-log: There are test failures.
> [ERROR]
> [ERROR] Please refer to C:\zproj\nifi-ijokarumawak\nifi-commons\nifi-write-ahead-log\target\surefire-reports for the individual test results.
> [ERROR] Please refer to dump files (if any exist) [date]-jvmRun[N].dump, [date].dumpstream and [date]-jvmRun[N].dumpstream.
> [ERROR] -> [Help 1]
> [ERROR]
> [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
> [ERROR] Re-run Maven using the -X switch to enable full debug logging.
> [ERROR]
> [ERROR] For more information about the errors and possible solutions, please read the following articles:
> [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
> [ERROR]
> [ERROR] After correcting the problems, you can resume the build with the command
> [ERROR] mvn <goals> -rf :nifi-write-ahead-log
```
I've put the entire output into a pastebin here: https://pastebin.com/Am3k3xn4
Is there something I'm missing or doing wrong?
---
[GitHub] nifi issue #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Cou...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/2750
Thanks @ijokarumawak! This has been merged to master.
---
[GitHub] nifi issue #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Cou...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2750
@mcgilman Thanks for reviewing. I've changed how Relationships are created.
---
[GitHub] nifi issue #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Cou...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2750
Thanks @mgroves for confirming that you can use username/password authentication, that helps a lot to finish review cycle!
As for the compatibility against older version of Couchbase Server, I tested this updated NiFi component against Couchbase Server 4.5 EE. I confirmed followings:
- A bucket without password can be accessed
- A bucket with password can be accessed with a bucket password using the existing user-defined-property configuration.
- What if a bucket with a password is accessed with a user name and password? If the user name is the bucket name and password is a bucket password, then it succeeds.
Couchbase Java SDK provides backward compatibility well, so NiFi can rely on that. The updated NiFi can access Couchbase 4.x and 5.x.
---
[GitHub] nifi pull request #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Exp...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2750#discussion_r195449260
--- Diff: nifi-nar-bundles/nifi-couchbase-bundle/nifi-couchbase-processors/src/main/java/org/apache/nifi/processors/couchbase/AbstractCouchbaseProcessor.java ---
@@ -39,54 +38,32 @@
import com.couchbase.client.core.CouchbaseException;
import com.couchbase.client.java.Bucket;
+import static org.apache.nifi.couchbase.CouchbaseConfigurationProperties.BUCKET_NAME;
+import static org.apache.nifi.couchbase.CouchbaseConfigurationProperties.COUCHBASE_CLUSTER_SERVICE;
+
/**
- * Provides common functionalities for Couchbase processors.
+ * Provides common functionality for Couchbase processors.
*/
public abstract class AbstractCouchbaseProcessor extends AbstractProcessor {
- public static final PropertyDescriptor DOCUMENT_TYPE = new PropertyDescriptor.Builder().name("Document Type")
- .description("The type of contents.")
- .required(true)
- .allowableValues(DocumentType.values())
- .defaultValue(DocumentType.Json.toString())
- .build();
-
- public static final PropertyDescriptor DOC_ID = new PropertyDescriptor.Builder().name("Document Id")
+ public static final PropertyDescriptor DOC_ID = new PropertyDescriptor.Builder()
+ .name("document-id")
+ .displayName("Document Id")
.description("A static, fixed Couchbase document id, or an expression to construct the Couchbase document id.")
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
- public static final Relationship REL_SUCCESS = new Relationship.Builder()
- .name("success")
- .description("All FlowFiles that are written to Couchbase Server are routed to this relationship.")
- .build();
- public static final Relationship REL_ORIGINAL = new Relationship.Builder()
- .name("original")
- .description("The original input file will be routed to this destination when it has been successfully processed.")
- .build();
- public static final Relationship REL_RETRY = new Relationship.Builder()
- .name("retry")
- .description("All FlowFiles that cannot written to Couchbase Server but can be retried are routed to this relationship.")
- .build();
- public static final Relationship REL_FAILURE = new Relationship.Builder()
- .name("failure")
- .description("All FlowFiles that cannot written to Couchbase Server and can't be retried are routed to this relationship.")
- .build();
-
- public static final PropertyDescriptor COUCHBASE_CLUSTER_SERVICE = new PropertyDescriptor.Builder().name("Couchbase Cluster Controller Service")
- .description("A Couchbase Cluster Controller Service which manages connections to a Couchbase cluster.")
- .required(true)
- .identifiesControllerService(CouchbaseClusterControllerService.class)
- .build();
+ public static final Relationship.Builder RELB_SUCCESS = new Relationship.Builder().name("success");
+ public static final Relationship.Builder RELB_ORIGINAL = new Relationship.Builder().name("original");
+ public static final Relationship.Builder RELB_RETRY = new Relationship.Builder().name("retry");
+ public static final Relationship.Builder RELB_FAILURE = new Relationship.Builder().name("failure");
- public static final PropertyDescriptor BUCKET_NAME = new PropertyDescriptor.Builder().name("Bucket Name")
- .description("The name of bucket to access.")
- .required(true)
- .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
- .defaultValue("default")
- .build();
+ public static final Relationship REL_ORIGINAL = RELB_ORIGINAL.build();
+ public static final Relationship REL_SUCCESS = RELB_SUCCESS.build();
+ public static final Relationship REL_RETRY = RELB_RETRY.build();
+ public static final Relationship REL_FAILURE = RELB_FAILURE.build();
--- End diff --
While the issue does not surface due to the way `getRelationships` is invoked, as `static` fields I believe these `Relationship` and `Relationship.Builder` variables are shared across any implementations of an `AbstractCouchbaseProcessor`. Because they are shared, when one implementation set's a description it would be reflected in any other implementation. With an approach like this, it probably makes sense to have these be not `static`.
---
[GitHub] nifi issue #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Cou...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2750
Hi @mgroves, first of all, thanks for jumping on the review process! Some unit tests fail due to environmental difference or race conditions. Does the error happen consistently? If so, the test might have issues on Windows. We have disabled several unit tests on Windows, and we may need to do the same with that particular one. However, the test has been there for about a year. I suspect the failure can occur occasionally. Anyway, since the test is not related to this PR, you can ignore it to proceed.
If the test failure persist, please add `-DskipTests` maven option, or `@Ignore` annotation to that test method.
---
[GitHub] nifi issue #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Cou...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2750
@mgroves In order to get this PR merged, we need a +1 from another NiFi committer. We need to wait for further review. Hope it will happen soon.
If you have time, it would be helpful to know how you feel about other enhancements, especially CouchbaseRecordLookupService. It may require you to get familiar with many NiFi topics such as distributed cache, Record data model and Schema registry.
I don't remember if I already shared, but actually I wrote a blog post (in Japanese, but we're living in a machine translation era) about improvements included in this PR. It can be used for the guest blog post if the content looks ok.
http://ijokarumawak.github.io/nifi/couchbase/2018/06/04/nifi-couchbase-update/
---
[GitHub] nifi pull request #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Exp...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2750#discussion_r195453378
--- Diff: nifi-nar-bundles/nifi-couchbase-bundle/nifi-couchbase-processors/src/main/java/org/apache/nifi/processors/couchbase/AbstractCouchbaseProcessor.java ---
@@ -39,54 +38,32 @@
import com.couchbase.client.core.CouchbaseException;
import com.couchbase.client.java.Bucket;
+import static org.apache.nifi.couchbase.CouchbaseConfigurationProperties.BUCKET_NAME;
+import static org.apache.nifi.couchbase.CouchbaseConfigurationProperties.COUCHBASE_CLUSTER_SERVICE;
+
/**
- * Provides common functionalities for Couchbase processors.
+ * Provides common functionality for Couchbase processors.
*/
public abstract class AbstractCouchbaseProcessor extends AbstractProcessor {
- public static final PropertyDescriptor DOCUMENT_TYPE = new PropertyDescriptor.Builder().name("Document Type")
- .description("The type of contents.")
- .required(true)
- .allowableValues(DocumentType.values())
- .defaultValue(DocumentType.Json.toString())
- .build();
-
- public static final PropertyDescriptor DOC_ID = new PropertyDescriptor.Builder().name("Document Id")
+ public static final PropertyDescriptor DOC_ID = new PropertyDescriptor.Builder()
+ .name("document-id")
+ .displayName("Document Id")
.description("A static, fixed Couchbase document id, or an expression to construct the Couchbase document id.")
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
- public static final Relationship REL_SUCCESS = new Relationship.Builder()
- .name("success")
- .description("All FlowFiles that are written to Couchbase Server are routed to this relationship.")
- .build();
- public static final Relationship REL_ORIGINAL = new Relationship.Builder()
- .name("original")
- .description("The original input file will be routed to this destination when it has been successfully processed.")
- .build();
- public static final Relationship REL_RETRY = new Relationship.Builder()
- .name("retry")
- .description("All FlowFiles that cannot written to Couchbase Server but can be retried are routed to this relationship.")
- .build();
- public static final Relationship REL_FAILURE = new Relationship.Builder()
- .name("failure")
- .description("All FlowFiles that cannot written to Couchbase Server and can't be retried are routed to this relationship.")
- .build();
-
- public static final PropertyDescriptor COUCHBASE_CLUSTER_SERVICE = new PropertyDescriptor.Builder().name("Couchbase Cluster Controller Service")
- .description("A Couchbase Cluster Controller Service which manages connections to a Couchbase cluster.")
- .required(true)
- .identifiesControllerService(CouchbaseClusterControllerService.class)
- .build();
+ public static final Relationship.Builder RELB_SUCCESS = new Relationship.Builder().name("success");
+ public static final Relationship.Builder RELB_ORIGINAL = new Relationship.Builder().name("original");
+ public static final Relationship.Builder RELB_RETRY = new Relationship.Builder().name("retry");
+ public static final Relationship.Builder RELB_FAILURE = new Relationship.Builder().name("failure");
- public static final PropertyDescriptor BUCKET_NAME = new PropertyDescriptor.Builder().name("Bucket Name")
- .description("The name of bucket to access.")
- .required(true)
- .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
- .defaultValue("default")
- .build();
+ public static final Relationship REL_ORIGINAL = RELB_ORIGINAL.build();
+ public static final Relationship REL_SUCCESS = RELB_SUCCESS.build();
+ public static final Relationship REL_RETRY = RELB_RETRY.build();
+ public static final Relationship REL_FAILURE = RELB_FAILURE.build();
--- End diff --
Additionally, should the visibility of these fields need to be public?
---
[GitHub] nifi issue #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Cou...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/2750
Thanks for the PR @ijokarumawak and thanks for assisting with the review @mgroves! I'll be happy to have a look and help get this merged.
---
[GitHub] nifi issue #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Cou...
Posted by mgroves <gi...@git.apache.org>.
Github user mgroves commented on the issue:
https://github.com/apache/nifi/pull/2750
Thank @ijokarumawak, I used -DskipTests and it built successfully for me on Windows. I tried out the PutCouchbaseKey operator with a CouchbaseClusterService using a username and password, and it works great.
I'm curious how it would work if someone wants to use a newer version of NiFi with Couchbase 4 or older. I'm not familiar enough with the NiFi architecture or project--will they have to use an old version of NiFi or is there someway they can keep using the older module?
---
[GitHub] nifi issue #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Expand Cou...
Posted by mgroves <gi...@git.apache.org>.
Github user mgroves commented on the issue:
https://github.com/apache/nifi/pull/2750
Great @ijokarumawak, thanks for that clarification. What are the next steps to getting this PR accepted? Do we need to just wait for further review, or is there anything else you need from me? When this PR is accepted, by the way, I'd love to have you write a guest blog post to talk about it and promote Nifi some more :)
---
[GitHub] nifi pull request #2750: NIFI-5054: Couchbase Authentication, NIFI-5257: Exp...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2750
---