turcsanyip commented on code in PR #7051:
URL: https://github.com/apache/nifi/pull/7051#discussion_r1159528894
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AbstractAWSProcessor.java:
##########
@@ -382,18 +382,6 @@ private String parseRegionForVPCE(String url, String
configuredRegion) {
}
}
- /**
- * Create client from the arguments
- * @param context process context
- * @param credentials static aws credentials
- * @param config aws client configuration
- * @return ClientType aws client
- *
- * @deprecated use {@link
AbstractAWSCredentialsProviderProcessor#createClient(ProcessContext,
AWSCredentialsProvider, ClientConfiguration)}
- */
- @Deprecated
- protected abstract ClientType createClient(final ProcessContext context,
final AWSCredentials credentials, final ClientConfiguration config);
-
protected ClientType getClient() {
return client;
}
Review Comment:
The abstract processor should not store the client directly, only in the
cache. It is quite confusing and error prone if some processors store the
client in the field, others in the cache while everybody can access both type
of client storage.
I would suggest making this `getClient()` method the entry point for the
client cache (instead of `createClient()`). In this case it needs to be
extended with `ProcessContext` parameter and also with `AwsClientDetails`.
Something like this:
```
protected ClientType getClient(final ProcessContext context, final
AwsClientDetails clientDetails) {
return getAwsClientFactory().getOrCreateClient(context,
clientDetails, this);
}
```
The processor implementation provides the `AwsClientDetails` object with the
AWS region (e.g. parsed from FlowFile attributes in case of S3 processors)
The abstract class could also provide a convenience method for processors
that do not have specific region handling:
```
protected ClientType getClient(final ProcessContext context) {
AwsClientDetails clientDetails = ... // create AwsClientDetails from
the region in the context
return getClient(context, clientDetails);
}
```
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AbstractAWSProcessor.java:
##########
@@ -441,20 +431,27 @@ protected void setClientAndRegion(final ProcessContext
context) {
* @param context The process context
* @return The created client
*/
- protected ClientType createClient(final ProcessContext context) {
- return createClient(context, getCredentials(context),
createConfiguration(context));
+ public ClientType createClient(final ProcessContext context,
AwsClientDetails awsClientDetails) {
+ AWSCredentials credentials = getCredentials(context);
+ final ClientType createdClient =
getAwsClientFactory().getOrCreateClient(context, credentials, awsClientDetails,
this);
+
+ setRegionAndInitializeEndpoint(awsClientDetails.getRegion(), context,
createdClient);
+ return createdClient;
+ }
Review Comment:
As I mentioned at `getClient()`, I would move the cache access to that
method and `createClient()` would only be responsible for creating the client
(as before; the difference is that now we need `AwsClientDetails` parameter
too).
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AbstractAWSProcessor.java:
##########
@@ -425,9 +413,11 @@ protected AWSCredentials getCredentials(final
PropertyContext context) {
@OnShutdown
public void onShutdown() {
- if ( getClient() != null ) {
+ if (getClient() != null) {
getClient().shutdown();
}
+
+
getAwsClientFactory().getClients().forEach(AmazonWebServiceClient::shutdown);
Review Comment:
If we do not store the client separately, only the cached clients need to be
shut down.
However, I noticed an old issue with this method: it is hooked on
`@OnShutdown`. I believe the intent was `@OnStopped`. Now it runs only at NiFi
shutdown which is mostly useless as the JVM will quit and client be destroyed.
Also, it shuts down the lastly used client only (clients recreated at
start/stop that are not shut down).
As it has not been an issue so far, I think we can simply delete this method
for now.
In order to implement the client shutdown properly, we should also handle
clients evicted from the cache too. It could be implemented in a separate jira.
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AwsClientProvider.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.processors.aws;
+
+import com.amazonaws.AmazonWebServiceClient;
+import com.amazonaws.ClientConfiguration;
+import com.amazonaws.auth.AWSCredentials;
+import com.amazonaws.auth.AWSCredentialsProvider;
+import org.apache.nifi.processor.ProcessContext;
+
+public interface AwsClientProvider<ClientType extends AmazonWebServiceClient> {
+ /**
+ * Create client from the arguments
+ * @param context process context
+ * @param credentials static AWS credentials
+ * @param config AWS client configuration
+ * @return ClientType AWS client
+ *
+ * @deprecated use {@link #createClient(ProcessContext,
AWSCredentialsProvider, ClientConfiguration) }
+ */
+ @Deprecated
+ ClientType createClient(final ProcessContext context, final AWSCredentials
credentials, final ClientConfiguration config);
+
+ /**
+ * Create AWS client using credentials provider. This is the preferred
method for creating AWS clients
+ * @param context process context
+ * @param credentialsProvider AWS credentials provider
+ * @param config AWS client configuration
+ * @return ClientType AWS client
+ */
+ ClientType createClient(final ProcessContext context, final
AWSCredentialsProvider credentialsProvider, final ClientConfiguration config);
Review Comment:
With moving the cache access logic to `getClient()`, this
`AwsClientProvider` interface can expose `createClient(ProcessContext,
AwsClientDetails)` only, which is the "original" / "main" method for creating
the client. In this case we can get rid of `AWSCredentials`,
`AWSCredentialsProvider` and `ClientConfiguration` in this interface (those are
rather implementation details in the abstract processors).
##########
nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AwsClientFactory.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.processors.aws;
+
+import com.amazonaws.AmazonWebServiceClient;
+import com.amazonaws.ClientConfiguration;
+import com.amazonaws.auth.AWSCredentials;
+import com.amazonaws.auth.AWSCredentialsProvider;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.nifi.processor.ProcessContext;
+
+public class AwsClientFactory<ClientType extends AmazonWebServiceClient> {
Review Comment:
As `AwsClientProvider` creates the client and this class handles the cache,
so I would rename it to `AwsClientCache`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
