turcsanyip commented on code in PR #7051: URL: https://github.com/apache/nifi/pull/7051#discussion_r1169156291
########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/test/java/org/apache/nifi/processors/aws/TestAwsClientCache.java: ########## @@ -0,0 +1,85 @@ +/* + * 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 static com.amazonaws.regions.Regions.US_EAST_1; +import static com.amazonaws.regions.Regions.US_WEST_2; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.amazonaws.AmazonWebServiceClient; +import com.amazonaws.regions.Region; +import com.amazonaws.regions.Regions; +import org.apache.nifi.processor.ProcessContext; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +public class TestAwsClientCache { + + @Mock + private ProcessContext contextMock; + @Mock + private AwsClientProvider<AmazonWebServiceClient> awsClientProviderMock; + @Mock + private AmazonWebServiceClient awsClientMock1; + @Mock + private AmazonWebServiceClient awsClientMock2; + private AwsClientCache<AmazonWebServiceClient> clientCache; + + @BeforeEach + public void setup() { + clientCache = new AwsClientCache<>(); + } + + @Test + public void testClientCreationNotNeededGetAwsTranslateJobStatusTest() { + final AwsClientDetails clientDetails = getClientDetails(US_WEST_2); + when(awsClientProviderMock.createClient(contextMock, clientDetails)).thenReturn(awsClientMock1); + final AmazonWebServiceClient client1 = clientCache.getOrCreateClient(contextMock, clientDetails, awsClientProviderMock); + + final AwsClientDetails newClientDetails = getClientDetails(US_WEST_2); + final AmazonWebServiceClient client2 = clientCache.getOrCreateClient(contextMock, newClientDetails, awsClientProviderMock); + verify(awsClientProviderMock, times(1)).createClient(eq(contextMock), any(AwsClientDetails.class)); + assertEquals(client1, client2); Review Comment: The two client objects should be the same by reference. So `assertSame()` could be used and it would describe the use case more explicitly.. ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AbstractAWSProcessor.java: ########## @@ -423,17 +408,28 @@ protected AWSCredentials getCredentials(final PropertyContext context) { return new AnonymousAWSCredentials(); } - @OnShutdown - public void onShutdown() { - if ( getClient() != null ) { - getClient().shutdown(); - } + @OnStopped + public void onStopped() { + getAwsClientCache().clearCache(); + } + + /** + * Creates an AWS service client from the context or returns an existing client from the cache + * @param context The process context + * @param awsClientDetails details of the AWS client + * @return The created client + */ + protected ClientType getClient(final ProcessContext context, AwsClientDetails awsClientDetails) { + return getAwsClientCache().getOrCreateClient(context, awsClientDetails, this); } - protected void setClientAndRegion(final ProcessContext context) { - final AWSConfiguration awsConfiguration = getConfiguration(context); - this.client = awsConfiguration.getClient(); - this.region = awsConfiguration.getRegion(); + protected ClientType getClient(final ProcessContext context) { + final AwsClientDetails awsClientDetails = new AwsClientDetails(getRegion(context)); + return getClient(context, awsClientDetails); + } + + protected AwsClientCache<ClientType> getAwsClientCache() { + return awsClientCache; Review Comment: `getAwsClientCache()` is not called or overridden in child classes. The method should be `private` or even the field could be accessed directly instead of the getter method. ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AbstractAWSProcessor.java: ########## @@ -446,15 +442,26 @@ protected ClientType createClient(final ProcessContext context) { } /** - * Parses and configures the client and region from the context. + * 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); + + /** + * Parses and configures the client from the context. * @param context The process context * @return The parsed configuration */ protected AWSConfiguration getConfiguration(final ProcessContext context) { - final ClientType client = createClient(context); - final Region region = getRegionAndInitializeEndpoint(context, client); - - return new AWSConfiguration(client, region); + final AwsClientDetails awsClientDetails = new AwsClientDetails(getRegion(context)); + final ClientType createdClient = getClient(context, awsClientDetails); + return new AWSConfiguration(createdClient, awsClientDetails.getRegion()); Review Comment: I'd suggest removing this method and the `AWSConfiguration` inner class. `onScheduled()` methods can call `getClient(ProcessContext)` directly which is more explicit and readable I believe. `verify()` methods should not call `getClient()` (directly or indirectly via `getConfiguration()`) because `getClient()` populates the cache with the created client. If the user modifies some properties after verification, the old client gets stuck in the cache and no new client is created with the changed property values. `verify()` methods can call `createClient(ProcessContext)` which does not access / initialize the cache. ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AwsClientCache.java: ########## @@ -0,0 +1,40 @@ +/* + * 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.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import org.apache.nifi.processor.ProcessContext; + +public class AwsClientCache<ClientType extends AmazonWebServiceClient> { + + private static final int MAXIMUM_CACHE_SIZE = 10; + + private final Cache<AwsClientDetails, ClientType> clientCache = Caffeine.newBuilder() + .maximumSize(MAXIMUM_CACHE_SIZE) + .build(); + + public ClientType getOrCreateClient(final ProcessContext context, final AwsClientDetails clientDetails, final AwsClientProvider<ClientType> provider) { + return clientCache.get(clientDetails, ignored -> provider.createClient(context, clientDetails)); + } + + public void clearCache() { + clientCache.cleanUp(); Review Comment: `Cache.invalidateAll()` needs to be called in order to remove the items and to clear the cache. `cleanUp()` just _"performs any pending maintenance operations"_. Not sure if `cleanUp()` is needed after `invalidateAll()`. For sure, we can use both. A unit test may be added to check the proper cleaning. -- 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
