feat(log-manager): Add configurable buffer interval for writting upload action to tlog #228
Conversation
| buffer.clear(); | ||
|
|
||
| logger.atDebug().log("Successfully flushed buffer '{}'", bufferName); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
It appears that your code handles a broad swath of exceptions in the catch block, potentially trapping dissimilar issues or problems that should not be dealt with at this point in the program.
| } | ||
|
|
||
| // Shutdown the scheduler | ||
| scheduler.shutdownNow(); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Using shutdownNow() causes abrupt termination of thread pool tasks, which can lead to resource leaks, data corruption, and incomplete operations as threads are forcefully interrupted mid-execution. To avoid these risks, first call shutdown() to reject new tasks, then use awaitTermination() with a timeout to allow existing tasks to complete. Check awaitTermination()'s return value and only use shutdownNow() as a last resort if graceful shutdown fails. See the following diffs on GitHub that make this code change: diff1, diff2
Learn more: ExecutorService in the Java Platform, Standard Edition 8 API Specification
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Configure with short intervals for faster testing | ||
| Map<String, Object> additionalConfig = new HashMap<>(); |
Check failure
Code scanning / CodeQL
Container contents are never accessed Error
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix the issue, we need to ensure that the additionalConfig map is either removed if it is genuinely unnecessary or its contents are accessed explicitly. If the setupKernel method uses the map, we should update the code to make this usage clear. For example, we can pass additionalConfig as an argument to setupKernel and verify its usage within the method.
| @@ -310,3 +310,3 @@ | ||
|
|
||
| setupKernel(tempDirectoryPath, "smallPeriodicIntervalUserComponentConfig.yaml"); | ||
| setupKernel(tempDirectoryPath, "smallPeriodicIntervalUserComponentConfig.yaml", additionalConfig); | ||
|
|
| // Shutdown the scheduler | ||
| scheduler.shutdown(); | ||
| try { | ||
| if (!scheduler.awaitTermination(5, TimeUnit.SECONDS)) { |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
These configs are flagged under GS-Tech Static config campaign and is considered to be a violation. Please migrate these configs
|
|
||
| logger.atInfo().log("Initializing config update buffers with interval: {} seconds", bufferIntervalSeconds); | ||
|
|
||
| // Buffer for processing files information |
There was a problem hiding this comment.
do we need separate buffers?
| // Create a named thread factory for better debugging | ||
| ThreadFactory namedThreadFactory = runnable -> { | ||
| Thread thread = new Thread(runnable, "PeriodicBuffer-" + bufferName + "-Flusher"); | ||
| thread.setDaemon(true); |
| return thread; | ||
| }; | ||
|
|
||
| this.scheduler = Executors.newSingleThreadScheduledExecutor(namedThreadFactory); |
There was a problem hiding this comment.
don't we have a ScheduledExecutorService in nucleus?
3 participants
Issue #, if available:
Description of changes:
Buffer:
Added configurable buffer interval seconds via the
bufferIntervalSecconfiguration parameter. The buffer accumulates log processing state updates and flushes them to config.tlog periodically.Implementation of Buffer:
PeriodicBuffer class uses ScheduledExecutorService to control the periodic flush action.
Implementation of Buffer on LogManager:
The buffer in LogManager is a duplication Map of both
lastComponentUploadedLogFileInstantMapandprocessingFilesInformation, the buffer is initialized from the constructor, taking the default valueperiodicUploadIntervalSec. When upload logs finished, the buffer copies the value from updated maps, which stores the Log information for each component. When the times up, then flush the current buffers to config.tlogWhy is this change necessary:
This change improves system performance by allowing customers to control how frequently log processing state is persisted to disk. By setting appropriate buffer intervals, customers can reduce disk I/O operations while ensuring log processing state is preserved at reasonable intervals.
How was this change tested:
Any additional information or context required to review the change:
Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.