egor-ryashin commented on a change in pull request #7038: Fix and document
concurrency of EventReceiverFirehose and TimedShutoffFirehose; Refine
concurrency specification of Firehose
URL: https://github.com/apache/incubator-druid/pull/7038#discussion_r260969913
##########
File path:
server/src/main/java/org/apache/druid/segment/realtime/firehose/EventReceiverFirehoseFactory.java
##########
@@ -280,52 +294,36 @@ private Thread createDelayedCloseExecutor()
// The closed = true is visible after close() because there is a
happens-before edge between
// delayedCloseExecutor.interrupt() call in close() and catching
InterruptedException below in this loop.
while (!closed) {
- Long closeTimeNs = null;
- Boolean dueToShutdownRequest = null;
+ // Reading idleCloseTimeNs and requestedShutdownTimeNs to locals
for absolute confidence that there
+ // couldn't be NPE in comparisons with nanoTime() below.
Long idleCloseTimeNs = this.idleCloseTimeNs;
- if (idleCloseTimeNs != null) {
- closeTimeNs = idleCloseTimeNs;
- dueToShutdownRequest = false;
- }
Long requestedShutdownTimeNs = this.requestedShutdownTimeNs;
- if (requestedShutdownTimeNs != null) {
- if (closeTimeNs == null || requestedShutdownTimeNs -
closeTimeNs <= 0) { // overflow-aware comparison
- closeTimeNs = requestedShutdownTimeNs;
- dueToShutdownRequest = true;
- }
- }
- // This is not possible unless there are bugs in the code of
EventReceiverFirehose. AssertionError could
- // have been thrown instead, but it doesn't seem to make a lot
of sense in a background thread. Instead,
- // we long the error and continue a loop after some pause.
- if (closeTimeNs == null) {
+ if (idleCloseTimeNs == null && requestedShutdownTimeNs == null) {
+ // This is not possible unless there are bugs in the code of
EventReceiverFirehose. AssertionError could
+ // have been thrown instead, but it doesn't seem to make a lot
of sense in a background thread. Instead,
+ // we long the error and continue a loop after some pause.
log.error(
"Either idleCloseTimeNs or requestedShutdownTimeNs must be
non-null. "
+ "Please file a bug at
https://github.com/apache/incubator-druid/issues";
);
- try {
- Threads.sleepFor(1, TimeUnit.MINUTES);
- }
- catch (InterruptedException ignore) {
- // Interruption is a wakeup, continue the loop
- }
- continue;
}
- long closeTimeoutNs = closeTimeNs - System.nanoTime();
- if (closeTimeoutNs <= 0) { // overflow-aware comparison
- if (dueToShutdownRequest) {
- log.info("Closing Firehose after a shutdown request");
- } else {
- log.info("Firehose has been idle for %d ms, closing.",
maxIdleTimeMillis);
- }
+ if (idleCloseTimeNs != null && idleCloseTimeNs -
System.nanoTime() <= 0) { // overflow-aware comparison
+ log.info("Closing Firehose after a shutdown request");
+ close();
+ } else if (requestedShutdownTimeNs != null &&
+ requestedShutdownTimeNs - System.nanoTime() <= 0) {
// overflow-aware comparison
+ log.info("Firehose has been idle for %d ms, closing.",
maxIdleTimeMillis);
close();
- return;
- } else {
- try {
- Threads.sleepFor(closeTimeoutNs, TimeUnit.NANOSECONDS);
- }
- catch (InterruptedException ignore) {
- // Interruption is a wakeup, continue the loop
- }
+ }
+ try {
+ // It is possible to write code that sleeps until the next the
next idleCloseTimeNs or
+ // requestedShutdownTimeNs, whatever is non-null and sooner,
but that's fairly complicated code. That
+ // complexity perhaps overweighs the minor inefficiency of
simply waking up every second.
+ // perhaps doesn't justify minor inefficiency of waking
Review comment:
> perhaps overweighs the minor inefficiency of simply waking up every second.
// perhaps doesn't justify minor inefficiency of waking
the last sentence is redundant
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
