-
Notifications
You must be signed in to change notification settings - Fork 404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OAK-10904: Close token refresh executor service after access token is no longer needed #1545
Conversation
@@ -74,7 +74,12 @@ public final class AzureUtilities { | |||
private static final long TOKEN_REFRESHER_DELAY = 1L; | |||
|
|||
private static final Logger log = LoggerFactory.getLogger(AzureUtilities.class); | |||
private static final ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(); | |||
|
|||
private static final ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(runnable -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making daemon is fine but is there no way we can call close on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not closing it properly could cause random issues in tests for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making daemon is fine but is there no way we can call close on this?
This function is part of the Util class and is being called from several places including other Util classes, closing this is not as straightforward, hence I opted for using daemon threads and adding a shutdown hook to close the executor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not closing it properly could cause random issues in tests for example
I did not face any issues in the test suite run, can you please elaborate so that I can handle the same? I guess the framework running test terminates the JVM and shutdown method is executed which will close this exectuor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation is problematic not only for tests but also for production use. If I understood correctly, every call to storageCredentialAccessTokenFrom(...)
will start a new refresh tasks. There is no mechanism to close this task when the token is no longer needed, so this can lead to the accumulation of many tasks that will keep on executing until the end of the lifetime of the JVM. This is problematic for several reasons:
- Memory leak of tasks
- Many unnecessary calls to Azure. This can cause issues with API request quotas in Azure.
- The execution of obsolete tasks may interfere with the ones that are still required, blocking them from being executed, which can lead to tokens not being refreshed on time.
A better design would be for the refresh service to be explicitly managed by the caller, requiring the caller to create and stop it when the token is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the current logic, moved the code to generate token to a concrete class, where each instance of a class will have a single access token and the same will be refresh periodically. No unnecessary call will be made to azure for token generation each time a blob is requested.
The caller of this class will be responsible for closing the token refresh executor service when the token is no longer needed.
}); | ||
return newArrayList(revisionIterator); | ||
} catch (Exception e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add an error log here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
# Conflicts: # oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/tool/ToolUtils.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new changes seem to fix the issues that I had identified with thread leaks, the new logic seems good, it leaves up to the caller the responsibility of creating and shutting down the token refresh service.
I added just a few minor comments about logging.
I only looked carefully at the threading logic, I did not go into detail about the other aspects of the PR.
throw new IllegalArgumentException( | ||
"Could not connect to the Azure Storage. Please verify if AZURE_CLIENT_ID, AZURE_CLIENT_SECRET and AZURE_TENANT_ID environment variables are correctly set!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should log the original exception or else propagate it, so that the cause of the problem is displayed on the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
throw new IllegalArgumentException( | ||
"Could not connect to the Azure Storage. Please verify if AZURE_SECRET_KEY environment variable is correctly set!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here.
log.error("Access token is null or empty"); | ||
throw new IllegalArgumentException("Could not connect to azure storage, access token is null or empty"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest removing the log.error from here, because the exception contains the same information.
… no longer needed (#1545) * OAK-10904: change token refresh executor to use daemon thread * OAK-10904: add log * OAK-10904: move storage credential method to concrete class and explicitly calling close to shutdown the executor * OAK-10904: wrap code for generating storage credentials in try catch * OAK-10904: minor changes * OAK-10904: refactor log
* OAK-10341 Tree store * OAK-10341 Tree store * OAK-10341 Tree store * OAK-10341 Tree store * OAK-10341 Tree store * OAK-10944: oak-auth-ldap: update commons-pool2 dependency to 2.12.0 (#1576) * OAK-10705: oak-standalone: update dependencies (#1411) Updated dependencies. * OAK-10905 | Add a configurable async checkpoint creator service (#1560) * OAK-10905 | Add license header to AsyncCheckpointService (#1579) * OAK-10848: commons: remove use of slf4j.event.Level in SystemPropertySupplier implementation (#1580) * OAK-10685: remove unused import of java.io.UnsupportedEncodingException * OAK-10949: blob-cloud, segment-aws: update aws SDK to 1.12.761 (dependencies reference vulnerable amazon ion-java version) (#1581) * OAK-10954: Update spotbugs plugin to 4.8.6.2 (#1588) * OAK-10959: webapp: update Tomcat dependency to 9.0.90 (#1589) * OAK-10960: blob-cloud, segment: update netty version to 4.1.111 (#1590) * OAK-10945: Remove usage of Guava Function interface (#1578) * OAK-10945: Remove usage of Guava Function interface (oak-upgrade) * OAK-10945: Remove usage of Guava Function interface (oak-store-spi) * OAK-10945: Remove usage of Guava Function interface (oak-it) * OAK-10945: Remove usage of Guava Function interface (oak-store-document) * OAK-10945: Remove usage of Guava Function interface (oak-store-composite) * OAK-10945: Remove usage of Guava Function interface (oak-segment-tar) * OAK-10945: Remove usage of Guava Function interface (oak-segment-azure) * OAK-10945: Remove usage of Guava Function interface (oak-security-spi) * OAK-10945: Remove usage of Guava Function interface (oak-security-search) * OAK-10945: Remove usage of Guava Function interface (oak-run-commons) * OAK-10945: Remove usage of Guava Function interface (oak-run) * OAK-10945: Remove usage of Guava Function interface (oak-lucene) * OAK-10945: Remove usage of Guava Function interface (oak-jcr) * OAK-10945: Remove usage of Guava Function interface (oak-exercise) * OAK-10945: Remove usage of Guava Function interface (oak-core-spi) * OAK-10945: Remove usage of Guava Function interface (oak-core) * OAK-10945: Remove usage of Guava Function interface (oak-commons) * OAK-10945: Remove usage of Guava Function interface (oak-blob-plugins) * OAK-10945: Remove usage of Guava Function interface (oak-blob-cloud, oak-blob-cloud-azure) * OAK-10945: Remove usage of Guava Function interface (oak-upgrade) - cleanup * OAK-10945: Remove usage of Guava Function interface (oak-store-spi) - cleanup * OAK-10945: Remove usage of Guava Function interface (oak-store-document) - cleanup * OAK-10945: Remove usage of Guava Function interface (oak-store-composite) - cleanup * OAK-10945: Remove usage of Guava Function interface (oak-segment-tar) - cleanup * OAK-10945: Remove usage of Guava Function interface (oak-lucene) - cleanup * OAK-10945: Remove usage of Guava Function interface (oak-jcr) - cleanup * OAK-10962: oak-solr-osgi: update zookeeper dependency to 3.9.2 (#1591) * Update build.yml to disable Sonar for now ...because of failures, as in https://github.com/apache/jackrabbit-oak/actions/runs/10021673018 * OAK-10965 - Make ConsoleIndexingReporter thread safe. (#1592) * OAK-6762: Convert oak-blob to OSGi R7 annotations (#1413) done * OAK-6773: Convert oak-store-composite to OSGi R7 annotations (#1489) done * OAK-10951 - Add a new configuration property (#1594) - "oak.indexer.persistedLinkedList.cacheSize" - sets the cache size of the PersistedLinkedList used to traverse the FFS. This controls the number FFS entries kept in memory. * OAK-10966 - Avoid object creation in PathUtils.isAncestor (#1596) * OAK-10964: bump nimbus-jose-jwt dependency to latest (#1593) * OAK-6773: Convert oak-store-composite to OSGi R7 annotations - fix line ends * OAK-10803 -- compress/uncompress property, disabled by default (#1526) Co-authored-by: pirlogea <[email protected]> * OAK-10974 and OAK-10869 : temporarily disabling flaky tests * OAK-10971 - Add a method to test if a path is a direct ancestor of another: PathUtils.isDirectAncestor() (#1598) * OAK-10803: fix NPE when Mongo is unavailable, remove '*' imports (#1601) * OAK-10976 - Avoid unnecessary call to PathUtils.getName in IndexDefinition (#1603) * OAK-10904: Close token refresh executor service after access token is no longer needed (#1545) * OAK-10904: change token refresh executor to use daemon thread * OAK-10904: add log * OAK-10904: move storage credential method to concrete class and explicitly calling close to shutdown the executor * OAK-10904: wrap code for generating storage credentials in try catch * OAK-10904: minor changes * OAK-10904: refactor log * OAK-10977 - Cleanup IndexDefinition class (#1604) - replace Guava usages with JDK equivalents - fix typos - improve formatting * OAK-10978 - Skip Azure compaction when there's not enough garbage in the repository (#1606) * OAK-10966 - Indexing job: create optimized version of PersistedLinkedList (#1595) * Revert "OAK-10966 - Indexing job: create optimized version of PersistedLinkedList (#1595)" This reverts commit 8a72ef8. * Revert "OAK-10978 - Skip Azure compaction when there's not enough garbage in the repository (#1606)" This reverts commit 5814638. * Revert "OAK-10977 - Cleanup IndexDefinition class (#1604)" This reverts commit ce5c7df. * Revert "OAK-10904: Close token refresh executor service after access token is no longer needed (#1545)" This reverts commit 632a15b. * Revert "OAK-10976 - Avoid unnecessary call to PathUtils.getName in IndexDefinition (#1603)" This reverts commit e411960. * Revert "OAK-10803: fix NPE when Mongo is unavailable, remove '*' imports (#1601)" This reverts commit 5dd6344. * Revert "OAK-10971 - Add a method to test if a path is a direct ancestor of another: PathUtils.isDirectAncestor() (#1598)" This reverts commit c416850. * Revert "OAK-10974 and OAK-10869 : temporarily disabling flaky tests" This reverts commit aefb990. * Revert "OAK-10803 -- compress/uncompress property, disabled by default (#1526)" This reverts commit 25792e7. * Revert "OAK-6773: Convert oak-store-composite to OSGi R7 annotations - fix line ends" This reverts commit 818317c. * Revert "OAK-10964: bump nimbus-jose-jwt dependency to latest (#1593)" This reverts commit 9528fdd. * Revert "OAK-10966 - Avoid object creation in PathUtils.isAncestor (#1596)" This reverts commit b37db4c. * Revert "OAK-10951 - Add a new configuration property (#1594)" This reverts commit ebefe01. * Revert "OAK-6773: Convert oak-store-composite to OSGi R7 annotations (#1489)" This reverts commit 0521c63. * Revert "OAK-6762: Convert oak-blob to OSGi R7 annotations (#1413)" This reverts commit 6c44805. * Revert "OAK-10965 - Make ConsoleIndexingReporter thread safe. (#1592)" This reverts commit 8b05cae. * Revert "Update build.yml to disable Sonar for now" This reverts commit 8db9183. * Revert "OAK-10962: oak-solr-osgi: update zookeeper dependency to 3.9.2 (#1591)" This reverts commit 95ed6c4. * Revert "OAK-10945: Remove usage of Guava Function interface (#1578)" This reverts commit 5d56db1. * Revert "OAK-10960: blob-cloud, segment: update netty version to 4.1.111 (#1590)" This reverts commit cd35db7. * Revert "OAK-10959: webapp: update Tomcat dependency to 9.0.90 (#1589)" This reverts commit 5fb55e0. * Revert "OAK-10954: Update spotbugs plugin to 4.8.6.2 (#1588)" This reverts commit b098fd8. * Revert "OAK-10949: blob-cloud, segment-aws: update aws SDK to 1.12.761 (dependencies reference vulnerable amazon ion-java version) (#1581)" This reverts commit cafbd29. * Revert "OAK-10685: remove unused import of java.io.UnsupportedEncodingException" This reverts commit 36cfcfa. * Revert "OAK-10848: commons: remove use of slf4j.event.Level in SystemPropertySupplier implementation (#1580)" This reverts commit ba530fe. * Revert "OAK-10905 | Add license header to AsyncCheckpointService (#1579)" This reverts commit 7f112fa. * Revert "OAK-10905 | Add a configurable async checkpoint creator service (#1560)" This reverts commit f12ccb1. * Revert "OAK-10705: oak-standalone: update dependencies (#1411)" This reverts commit dc10a12. * Revert "OAK-10944: oak-auth-ldap: update commons-pool2 dependency to 2.12.0 (#1576)" This reverts commit 3d6d009. * OAK-10341 Tree store (bugfix for loggers) * OAK-10341 Tree store (PipelinedTreeStoreStrategy.java) * OAK-10341 Tree store (tests) * OAK-10341 Tree store (use less memory) * OAK-10341 Tree store (fix memory cache calculation) * OAK-10341 Tree store (blob prefetch) * OAK-10341 Tree store (blob prefetch) * OAK-10341 Tree store (blob prefetch) * OAK-10341 Tree store (node prefetch) * OAK-10341 Tree store (incremental) * OAK-10341 Tree store (pack files) * OAK-10341 Tree store (pack files) * OAK-10341 Tree store (tests) * OAK-10341 Tree store (incremental) * OAK-10341 Tree store (compress) * OAK-10341 Tree store (traverse included paths) * OAK-10341 Tree store * OAK-10341 Tree store * OAK-10341 Tree store * OAK-10341 Tree store * OAK-10341 Tree store (use same blob prefetching configuration as for FlatFileStore) * OAK-10341 Tree store (javadocs) * OAK-10341 Tree store (code review) --------- Co-authored-by: Julian Reschke <[email protected]> Co-authored-by: mbaedke <[email protected]> Co-authored-by: nit0906 <[email protected]> Co-authored-by: Nuno Santos <[email protected]> Co-authored-by: Tushar <[email protected]> Co-authored-by: ionutzpi <[email protected]> Co-authored-by: pirlogea <[email protected]> Co-authored-by: Stefan Egli <[email protected]> Co-authored-by: Andrei Dulceanu <[email protected]>
No description provided.