Migrate GcsOptions#getExecutorService to an unbounded ScheduledExecutorService #23545
Conversation
lukecwik
force-pushed
the
21368
branch
2 times, most recently
from
603f030
to
89819d7
Compare
lukecwik
changed the title
|
R: @johnjcasey |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
…orService This is a preliminary piece for issue apache#21368
|
Fixed spotbugs/fb warnings |
|
Run Java PreCommit |
|
I think this is good, but it is pretty complex. |
| threadFactoryBuilder.setThreadFactory(MoreExecutors.platformThreadFactory()); | ||
| threadFactoryBuilder.setDaemon(true); | ||
|
|
||
| this.threadPoolExecutor = |
There was a problem hiding this comment.
Given that we are basically wrapping a thread pool executor here, does it make sense to allow users to configure different TPEs?
There was a problem hiding this comment.
Eventually yes, but as an initial implementation I decided against it since it is easier to add features then remove them in the future.
| @Override | ||
| public Void call() throws Exception { | ||
| while (true) { | ||
| synchronized (tasks) { |
There was a problem hiding this comment.
because we sync on tasks here, is it possible for us to have late execution of a task that is intended to be submitted during this lock?
There was a problem hiding this comment.
We only sync on tasks until we can pull out an entry and submit it to the executor.
Note that tasks.wait() frees the monitor lock allowing other threads to synchronize on the tasks monitor so the amount of time in the critical section is relatively short.
| } | ||
|
|
||
| @Override | ||
| public void execute(Runnable command) { |
There was a problem hiding this comment.
I assume we can't use an @nonnull annotation because we are implementing a method?
There was a problem hiding this comment.
@nonnull is the default by the way, we have to explicitly say something is @nullable.
Also @nonnull only adds checking for those that use that tooling when compiling their code.
The explicit check makes sure command isn't null for all those that don't use findbugs/spotbugs and to match the ScheduledExecutorService method contracts.
|
LTGM |
This is a preliminary piece for issue #21368
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.