NIFI-10752: Implement GetCouchbase and PutCouchbase processors with SDK3 #10031
Conversation
There was a problem hiding this comment.
Thanks for putting together new support for Couchbase @mark-bathori!
I noted a couple initial recommendations, and I would like to take a closer look soon.
One other general note, did you consider creating an integration test using Testcontainers? That could be very useful for verifying behavior against a running instance of Couchbase.
|
|
||
| public interface CouchbaseConnectionService extends ControllerService { | ||
|
|
||
| PropertyDescriptor COUCHBASE_CLUSTER_SERVICE = new PropertyDescriptor.Builder() |
There was a problem hiding this comment.
Including Property Descriptors in a service interface definition is generally poor practice, because it is not part of the Controller Service contract. I recommend removing this Descriptor from the interface.
There was a problem hiding this comment.
Yes it doesn't seem right to put a Property Descriptors into this interface, the implementations will need to use a cluster service and somehow I wanted to avoid the duplication, will rework this.
| } | ||
|
|
||
| public enum ErrorHandlingStrategy { | ||
| Rollback, Failure, Retry |
There was a problem hiding this comment.
In general, enum values should be uppercased. Is there a reason for this naming convention?
There was a problem hiding this comment.
They should be uppercase, I'll fix them.
|
|
||
| import static org.apache.nifi.services.couchbase.exception.CouchbaseErrorHandler.ErrorHandlingStrategy.Failure; | ||
|
|
||
| public class CouchbaseErrorHandler { |
There was a problem hiding this comment.
Is there a reason for including this class in the API module? It seems like it should be located in one of the implementation modules, or in a shared module if needed.
There was a problem hiding this comment.
The idea here is to create a common class for error handling. SDK-specific implementations will then determine which exceptions are considered fatal, recoverable, or other categories by adding the proper mapping to this class. Based on that, it felt like the proper place for this class was alongside other classes used in the API interfaces.
Summary
This PR introduces an architectural pattern similar to the Kafka bundle. Its main idea is to decouple top-level Couchbase processors and services from specific SDK dependencies. This is achieved by introducing a Controller Service layer that directly implements the service API methods based on a selected SDK. The advantage of this approach is that if a new Couchbase SDK is released, we avoid duplicating existing top-level processors and services. Instead, a new service bundle can be created to implement the new SDK, and components can simply be reconfigured to use the updated service.
NIFI-10752
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation