-
Notifications
You must be signed in to change notification settings - Fork 62
refactor(flagd): Replace Thread.sleep with ScheduledExecutorService in SyncStreamQueueSource #1660
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
base: main
Are you sure you want to change the base?
Conversation
…d of Thread.sleep Co-authored-by: aepfli <9987394+aepfli@users.noreply.github.com>
Co-authored-by: aepfli <9987394+aepfli@users.noreply.github.com>
|
/gemini review |
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.
Code Review
This pull request effectively refactors the SyncStreamQueueSource to use a ScheduledExecutorService for non-blocking retry backoff, replacing the blocking Thread.sleep(). This is a solid improvement for scalability and responsiveness. The implementation of the scheduler and its shutdown sequence is well-handled. However, I've identified a few areas for improvement. There is some duplicated code for the scheduler initialization which could be refactored for better maintainability. More importantly, the refactoring has removed specific handling for InterruptedException, which is now caught by a general Exception handler. This is a regression that could affect graceful shutdown, and I've provided suggestions to restore correct interruption handling. I also found a minor issue with a potentially confusing log message in the new retry logic.
...e/contrib/providers/flagd/resolver/process/storage/connector/sync/SyncStreamQueueSource.java
Show resolved
Hide resolved
...e/contrib/providers/flagd/resolver/process/storage/connector/sync/SyncStreamQueueSource.java
Show resolved
Hide resolved
...e/contrib/providers/flagd/resolver/process/storage/connector/sync/SyncStreamQueueSource.java
Outdated
Show resolved
Hide resolved
...e/contrib/providers/flagd/resolver/process/storage/connector/sync/SyncStreamQueueSource.java
Show resolved
Hide resolved
fc11ade to
b2dd03a
Compare
| return Executors.newSingleThreadScheduledExecutor(r -> { | ||
| Thread t = new Thread(r, "flagd-sync-retry-scheduler"); | ||
| t.setDaemon(true); | ||
| return t; | ||
| }); |
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.
Don't we have a helper method for that?
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.
do we? and where and why?
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.
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
b2dd03a to
4704a0d
Compare
SyncStreamQueueSource.observeSyncStream()usesThread.sleep()for retry backoff, blocking the thread and limiting scalability. This replaces it with non-blocking scheduled execution.Changes
ScheduledExecutorServicefor retry scheduling with named daemon thread (flagd-sync-retry-scheduler)scheduler.schedule()that re-invokesobserveSyncStream()after backoff delayRejectedExecutionExceptioncatch and properawaitTermination()with interrupt handlingFollows the same pattern already used in
FlagdProvider.javafor error task scheduling.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
checkstyle.org/opt/hostedtoolcache/CodeQL/2.23.3/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.23.3/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.23.3/x64/codeql/xml/tools/xml-extractor.jar --fileList=/home/REDACTED/work/java-sdk-contrib/.codeql-scratch/dbs/java/working/files-to-index13214004798705728053.list --sourceArchiveDir=/home/REDACTED/work/java-sdk-contrib/.codeql-scratch/dbs/java/src --outputDir=/home/REDACTED/work/java-sdk-contrib/.codeql-scratch/dbs/java/trap/java(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.