NIFI-10752: Add Couchbase 3.x components #8839

mattyb149 · 2024-05-15T17:25:14Z

Summary

NIFI-10752 This PR adds the Couchbase components back into NiFi, but using the Couchbase 3.x SDK and added to the assembly via a profile include-couchbase. The Jira has a sample flow definition attached.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using mvn clean install -P contrib-check
- JDK 21

UI Contributions

NiFi is modernizing its UI. Any contributions that update the current UI also need to be implemented in the new UI.

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

pvillard31 · 2024-05-17T17:50:42Z

nifi-assembly/pom.xml

@@ -1072,6 +1072,30 @@ language governing permissions and limitations under the License. -->
 </dependency>
 </dependencies>
 </profile>
+ <profile>
+ <id>include-couchbase</id>
+ <!-- This profile handles the inclusion of nifi-sql-reporting artifacts. -->


Suggested change

pvillard31 · 2024-05-17T17:53:59Z

...fi-couchbase-processors/src/main/java/org/apache/nifi/couchbase/CouchbaseClusterService.java

+@DynamicProperty(name = "Bucket Password for BUCKET_NAME", value = "bucket password",
+ description = "Specify bucket password if necessary." +
+ " Couchbase Server 5.0 or later should use 'User Name' and 'User Password' instead.")
+@DeprecationNotice(reason = "This component is deprecated and will be removed in NiFi 2.x.")


Suggested change

@DeprecationNotice(reason = "This component is deprecated and will be removed in NiFi 2.x.")

pvillard31 · 2024-05-17T17:54:32Z

...hbase-processors/src/main/java/org/apache/nifi/couchbase/CouchbaseKeyValueLookupService.java

+@Tags({"lookup", "enrich", "key", "value", "couchbase"})
+@CapabilityDescription("Lookup a string value from Couchbase Server associated with the specified key."
+ + " The coordinates that are passed to the lookup must contain the key 'key'.")
+@DeprecationNotice(reason = "This component is deprecated and will be removed in NiFi 2.x.")


Suggested change

@DeprecationNotice(reason = "This component is deprecated and will be removed in NiFi 2.x.")

pvillard31 · 2024-05-17T17:55:03Z

...uchbase-processors/src/main/java/org/apache/nifi/couchbase/CouchbaseRecordLookupService.java

+@Tags({"lookup", "enrich", "couchbase"})
+@CapabilityDescription("Lookup a record from Couchbase Server associated with the specified key."
+ + " The coordinates that are passed to the lookup must contain the key 'key'.")
+@DeprecationNotice(reason = "This component is deprecated and will be removed in NiFi 2.x.")


Suggested change

@DeprecationNotice(reason = "This component is deprecated and will be removed in NiFi 2.x.")

pvillard31 · 2024-05-17T17:56:22Z

...rocessors/src/main/java/org/apache/nifi/processors/couchbase/AbstractCouchbaseProcessor.java

+ .name("document-id")
+ .displayName("Document Id")


I believe we have gave up on using "machine friendly" names and the "new consensus" is to have name and display name configured with the same value.

Agreed, all properties should be changed to use the name() without a different displayName().

joewitt · 2024-05-17T18:20:33Z

nifi-extension-bundles/nifi-couchbase-bundle/nifi-couchbase-services-api/pom.xml

+ <dependencies>
+ <dependency>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi-api</artifactId>


this should not be declared. The parentage already has a dependency on nifi-api and it is provided. See nifi-bom for things which no matter what are already 'dependencies' of any nifi extension.

joewitt · 2024-05-17T18:21:18Z

...ouchbase-processors/src/test/java/org/apache/nifi/couchbase/TestCouchbaseMapCacheClient.java

+ private final Serializer<String> stringSerializer = (value, output) -> output.write(value.getBytes(StandardCharsets.UTF_8));
+ private final Deserializer<String> stringDeserializer = input -> new String(input, StandardCharsets.UTF_8);
+
+ // TODO: Add more tests


Let's either add them or get rid of this.

joewitt · 2024-05-17T18:22:50Z

nifi-extension-bundles/nifi-couchbase-bundle/nifi-couchbase-processors/pom.xml

+ <dependencies>
+ <dependency>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi-api</artifactId>


this should not be declared. The parentage already has a dependency on nifi-api and it is provided. See nifi-bom for things which no matter what are already 'dependencies' of any nifi extension.

joewitt · 2024-05-17T18:36:16Z

...xtension-bundles/nifi-couchbase-bundle/nifi-couchbase-nar/src/main/resources/META-INF/NOTICE

+
+The following binary components are provided under the Apache Software License v2
+
+ (ASLv2) Couchbase Java SDK


This is not accurate. It is not included.

joewitt · 2024-05-17T18:36:22Z

...xtension-bundles/nifi-couchbase-bundle/nifi-couchbase-nar/src/main/resources/META-INF/NOTICE

+ Couchbase Java SDK
+ Copyright 2014 Couchbase, Inc.
+
+ (ASLv2) RxJava


This is not accurate. It is not included.

joewitt · 2024-05-17T18:40:46Z

...fi-couchbase-processors/src/main/java/org/apache/nifi/couchbase/CouchbaseClusterService.java

+ .name("user-name")
+ .displayName("User Name")
+ .description("The user name to authenticate NiFi as a Couchbase client." +
+ " This configuration can be used against Couchbase Server 5.0 or later" +


We're using the couchbase client 3 which appears to be for Couchbase 7 or later. So this comment should be changed. And that compatibility detail should be shared elsewhere.

https://docs.couchbase.com/java-sdk/current/project-docs/compatibility.html#couchbase-versionsdk-version-matrix

joewitt · 2024-05-17T18:40:52Z

...fi-couchbase-processors/src/main/java/org/apache/nifi/couchbase/CouchbaseClusterService.java

+ .name("user-password")
+ .displayName("User Password")
+ .description("The user password to authenticate NiFi as a Couchbase client." +
+ " This configuration can be used against Couchbase Server 5.0 or later" +


We're using the couchbase client 3 which appears to be for Couchbase 7 or later. So this comment should be changed. And that compatibility detail should be shared elsewhere.

https://docs.couchbase.com/java-sdk/current/project-docs/compatibility.html#couchbase-versionsdk-version-matrix

joewitt · 2024-05-17T18:43:55Z

...main/resources/docs/org.apache.nifi.couchbase.CouchbaseMapCacheClient/additionalDetails.html

+
+<h3>Requirements</h3>
+
+<h4>Couchbase Server 4.0 or higher is required for some operation using N1QL</h4>


This whole section does not look correct/true from my read of

https://docs.couchbase.com/java-sdk/current/project-docs/compatibility.html#couchbase-versionsdk-version-matrix

exceptionfactory · 2024-05-17T18:35:35Z

...hbase-processors/src/main/java/org/apache/nifi/couchbase/AbstractCouchbaseLookupService.java

+public class AbstractCouchbaseLookupService extends AbstractControllerService {
+
+ protected static final String KEY = "key";
+ protected static final Set<String> REQUIRED_KEYS = Collections.unmodifiableSet(Stream.of(KEY).collect(Collectors.toSet()));


It looks like this can be simplified to Set.of(KEY)

If we don't intend to backport this I can change it, or I guess just issue a separate PR in that case and change it back.

exceptionfactory · 2024-05-17T18:37:30Z

...fi-couchbase-processors/src/main/java/org/apache/nifi/couchbase/CouchbaseClusterService.java

+ " supporting Roll-Based Access Control.")
+ .required(false)
+ .sensitive(true)
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)


This should be removed following general conventions against environment-based sensitive properties.

Suggested change

.expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)

exceptionfactory · 2024-05-17T18:37:54Z

...fi-couchbase-processors/src/main/java/org/apache/nifi/couchbase/CouchbaseClusterService.java

+ private static final List<PropertyDescriptor> properties;
+
+ static {
+ final List<PropertyDescriptor> props = new ArrayList<>();
+ props.add(CONNECTION_STRING);
+ props.add(USER_NAME);
+ props.add(USER_PASSWORD);
+
+ properties = Collections.unmodifiableList(props);
+ }


This can be condensed using List.of()

exceptionfactory · 2024-05-17T18:39:05Z

...fi-couchbase-processors/src/main/java/org/apache/nifi/couchbase/CouchbaseClusterService.java

+ final boolean isUserNameSet = context.getProperty(USER_NAME).isSet();
+ final boolean isUserPasswordSet = context.getProperty(USER_PASSWORD).isSet();
+ if ((isUserNameSet && !isUserPasswordSet) || (!isUserNameSet && isUserPasswordSet)) {
+ results.add(new ValidationResult.Builder()
+ .subject("User Name and Password")
+ .explanation("Both User Name and Password are required to use.")
+ .build());
+ }


This could be removed and replaced with an Authentication Strategy property, where Username and Password are both required with a selected strategy.

Turns out Couchbase 5.0 removed bucket passwords so I'll just remove the user-defined properties and make these properties required.

exceptionfactory · 2024-05-17T18:39:33Z

...fi-couchbase-processors/src/main/java/org/apache/nifi/couchbase/CouchbaseClusterService.java

+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .dynamic(true)
+ .sensitive(true)
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)


Suggested change

.expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)

exceptionfactory · 2024-05-17T18:45:13Z

...main/resources/docs/org.apache.nifi.couchbase.CouchbaseMapCacheClient/additionalDetails.html

+
+<h4>Couchbase Server 4.0 or higher is required for some operation using N1QL</h4>
+
+Following cache operations require N1QL query, thus you need to deploy Couchbase Server 4.0 or higher for those operations. However, as of this writing (May 2017) there are only few processors using these operations. Most cache APIs are implemented using document id lookup and should work with older version of Couchbase Server.


This description should be updated to reflect more recent behavior.

exceptionfactory · 2024-05-17T18:45:25Z

...ouchbase-processors/src/test/java/org/apache/nifi/couchbase/TestCouchbaseClusterService.java

+ System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");
+ System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");
+ System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.couchbase.PutCouchbaseKey", "debug");
+ System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.couchbase.CouchbaseClusterService", "debug");
+ System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.couchbase.TestCouchbaseClusterService", "debug");


This settings should be removed.

exceptionfactory · 2024-05-17T18:45:53Z

...le/nifi-couchbase-processors/src/test/java/org/apache/nifi/couchbase/TestCouchbaseUtils.java

+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class TestCouchbaseUtils {


This class should be removed, or changed to use Testcontainers.

exceptionfactory · 2024-05-17T18:46:08Z

...hbase-processors/src/test/java/org/apache/nifi/processors/couchbase/TestGetCouchbaseKey.java

+ System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");
+ System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");
+ System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.couchbase.GetCouchbaseKey", "debug");
+ System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.couchbase.TestGetCouchbaseKey", "debug");


Suggested change

System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");

System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");

System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.couchbase.GetCouchbaseKey", "debug");

System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.couchbase.TestGetCouchbaseKey", "debug");

exceptionfactory · 2024-05-17T18:46:39Z

...hbase-processors/src/test/java/org/apache/nifi/processors/couchbase/TestPutCouchbaseKey.java

+ System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");
+ System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");
+ System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.couchbase.PutCouchbaseKey", "debug");
+ System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.couchbase.TestPutCouchbaseKey", "debug");


Suggested change

System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "info");

System.setProperty("org.slf4j.simpleLogger.showDateTime", "true");

System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.couchbase.PutCouchbaseKey", "debug");

System.setProperty("org.slf4j.simpleLogger.log.org.apache.nifi.processors.couchbase.TestPutCouchbaseKey", "debug");

joewitt · 2024-05-17T18:49:34Z

...les/nifi-couchbase-bundle/nifi-couchbase-services-api-nar/src/main/resources/META-INF/NOTICE

+ Couchbase Java SDK
+ Copyright 2014 Couchbase, Inc.
+
+ (ASLv2) RxJava


I dont see any evidence to support RxJava being referred to here.

We do have reactor-core but it is ASLv2 with no apparent copyright claim we need to carry.

https://github.com/reactor/reactor-core

joewitt · 2024-05-17T18:56:07Z

nifi-extension-bundles/nifi-couchbase-bundle/nifi-couchbase-processors/pom.xml

+ </dependency>
+ <dependency>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi-mock</artifactId>


this dependency is already provided by the parentage. It should not be declared.

joewitt · 2024-05-17T18:57:16Z

nifi-extension-bundles/nifi-couchbase-bundle/nifi-couchbase-processors/pom.xml

+ <version>2.0.0-SNAPSHOT</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>


This POM does not declare the couchbase client yet the couchbase processors have a direct dependency on the couchbase client code. It should be explicitly declared and not simply relied upon to carry through from the transitive depedency.

joewitt · 2024-05-17T19:16:35Z

nifi-extension-bundles/nifi-couchbase-bundle/nifi-couchbase-processors/pom.xml

+ <artifactId>nifi-distributed-cache-client-service-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>


since this depends on commons-lang3 and that is already part of the nifi-standard-shared-bundle you could switch to that instead of standard services as the parent for these bundles and avoid having to carry another copy of commons-lang3. That would be a change in the parent pom reference currently set in this modules bundle and there is one less dependency but most importantly not risking getting them out of sync

joewitt · 2024-05-17T19:21:34Z

...les/nifi-couchbase-bundle/nifi-couchbase-services-api-nar/src/main/resources/META-INF/NOTICE

@@ -0,0 +1,21 @@
+nifi-couchbase-services-api-nar
+Copyright 2014-2024 The Apache Software Foundation
+


there is also reactive-streams in the build but not referenced. It is MIT licensed which means it needs to be copied into a LICENSE file and carried forward as appropriate to the assembly LICENSE too if not already.

https://github.com/reactive-streams/reactive-streams-jvm/blob/v1.0.4/LICENSE

joewitt · 2024-05-17T19:33:41Z

@mattyb149 Ok thanks for offering a PR that returns Couchbase components to the codebase.

However, given the rapid (perhaps record setting) pace by which you received feedback from 3 different PMC members you can see there is a lot of emphasis on increasing the quality of such contributions. It was deprecated in 1.x and removed in 2.x for what amounts of lack of maintenance and this new PR does not change that nature. There is clearly much of this which is simply copy and paste from the previous state. While there are a lot of comments all of them look actually very easily addressed.

They do however call into question the level of consideration that went into creating the newly offered couchbase components in the first place. L&N is important to get right and review. Avoiding duplicative dependencies in the chain is important for maintenance, behavior, and build size. Bumping from the older clients to the newer clients implies these components were considered for new compability implications with Couchbase server/etc.. By re-introducing these components to the codebase the presumption is that you as the author considered these things. If not, it is best to avoid re-introducing until such time that can be done because otherwise we're at the previous position again in terms of lack of maintenance.

Thanks

mattyb149 added 2 commits May 15, 2024 16:44

NIFI-10752: Add Couchbase 3.x components

0524756

NIFI-10752: Comply with new Checkstyle rules

517949e

mattyb149 force-pushed the NIFI-10752 branch from db9a4c9 to 517949e Compare May 15, 2024 21:04

pvillard31 requested changes May 17, 2024

View reviewed changes

joewitt reviewed May 17, 2024

View reviewed changes

exceptionfactory requested changes May 17, 2024

View reviewed changes

joewitt reviewed May 17, 2024

View reviewed changes

dan-s1 mentioned this pull request May 20, 2024

[NIFI-13082] Created SplitPcap processor, Pcap supporting class, and … #8691

Open

10 tasks

mattyb149 marked this pull request as draft May 20, 2024 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-10752: Add Couchbase 3.x components #8839

NIFI-10752: Add Couchbase 3.x components #8839

mattyb149 commented May 15, 2024

pvillard31 May 17, 2024

pvillard31 May 17, 2024

pvillard31 May 17, 2024

pvillard31 May 17, 2024

pvillard31 May 17, 2024

exceptionfactory May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

exceptionfactory May 17, 2024

mattyb149 May 20, 2024

exceptionfactory May 17, 2024

exceptionfactory May 17, 2024

exceptionfactory May 17, 2024

mattyb149 May 20, 2024

exceptionfactory May 17, 2024

exceptionfactory May 17, 2024

exceptionfactory May 17, 2024

exceptionfactory May 17, 2024

exceptionfactory May 17, 2024

exceptionfactory May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt May 17, 2024

joewitt commented May 17, 2024

	<!-- This profile handles the inclusion of nifi-sql-reporting artifacts. -->
	<!-- This profile handles the inclusion of nifi-couchbase artifacts. -->


		The following binary components are provided under the Apache Software License v2

		(ASLv2) Couchbase Java SDK


		<h3>Requirements</h3>

		<h4>Couchbase Server 4.0 or higher is required for some operation using N1QL</h4>


		<h4>Couchbase Server 4.0 or higher is required for some operation using N1QL</h4>

		Following cache operations require N1QL query, thus you need to deploy Couchbase Server 4.0 or higher for those operations. However, as of this writing (May 2017) there are only few processors using these operations. Most cache APIs are implemented using document id lookup and should work with older version of Couchbase Server.

		@@ -0,0 +1,21 @@
		nifi-couchbase-services-api-nar
		Copyright 2014-2024 The Apache Software Foundation

NIFI-10752: Add Couchbase 3.x components #8839

Are you sure you want to change the base?

NIFI-10752: Add Couchbase 3.x components #8839

Conversation

mattyb149 commented May 15, 2024

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

UI Contributions

Licensing

Documentation

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

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

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

Choose a reason for hiding this comment

joewitt commented May 17, 2024