You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/10/02 21:10:04 UTC
[GitHub] [druid] maytasm opened a new pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
maytasm opened a new pull request #10467:
URL: https://github.com/apache/druid/pull/10467
Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
### Description
When wrappingIterator.close() is called in the shutdown executor (of TimedShutoffInputSourceReader#decorateShutdownTimeout), the first thing it does is call exec::shutdownNow. This is because the Closer close closeables in LIFO order. Thus, the shutdown executor shutdowns itself first before shutting down the all the other registered closeable, which means that the closing of the other closeable may be interrupted, and the closing doesn't finish. This leave the closeable orphaned and running forever.
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r499879272
##########
File path: extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
##########
@@ -174,7 +174,7 @@ public Long getPosition(StreamPartition<Integer> partition)
}
@Override
- public void close()
+ public synchronized void close()
Review comment:
Please add a Javadoc explaining why it's synchronized.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] stale[bot] commented on pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10467:
URL: https://github.com/apache/druid/pull/10467#issuecomment-746016389
This issue is no longer marked as stale.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
maytasm commented on pull request #10467:
URL: https://github.com/apache/druid/pull/10467#issuecomment-746016320
Will address the remaining comments in the next week or so
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r500524612
##########
File path: extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
##########
@@ -174,7 +174,7 @@ public Long getPosition(StreamPartition<Integer> partition)
}
@Override
- public void close()
+ public synchronized void close()
Review comment:
Usually Kafka consumer is closed via `wakeup` method which is thread-safe and can be called concurrently with the other methods in KafkaConsumer. If one thread is busy in `poll` and another thread tries to `close`, it will likely result in an exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r499909858
##########
File path: extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
##########
@@ -174,7 +174,7 @@ public Long getPosition(StreamPartition<Integer> partition)
}
@Override
- public void close()
+ public synchronized void close()
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r499102357
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/TimedShutoffInputSourceReader.java
##########
@@ -68,8 +68,10 @@ public TimedShutoffInputSourceReader(InputSourceReader delegate, DateTime shutof
)
{
final Closer closer = Closer.create();
- closer.register(delegateIterator);
+ // We must register the shutdownNow method of the shutdown executor first. This will cause the shutdown executor
+ // to be closed last and ensure other resources which depend on it can close successfully.
closer.register(exec::shutdownNow);
Review comment:
Hmm, a few comments about this:
1. It's kind of poor form to close an executor from within a thread running in that same executor, which is what's happening here. Switching the order is a bit better, but it's still sort of like the picture below.
2. The record supplier's close function doesn't protect against multiple closes, and neither do Closers, so the record supplier might be closed multiple times or even concurrently. We should include something to ensure it will be OK if that happens.
3. Any ideas about how to test this? We should have testing for this sort of bug.
---
A photo of an executor being closed from a thread inside that same executor:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r499808488
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/TimedShutoffInputSourceReader.java
##########
@@ -68,8 +68,10 @@ public TimedShutoffInputSourceReader(InputSourceReader delegate, DateTime shutof
)
{
final Closer closer = Closer.create();
- closer.register(delegateIterator);
+ // We must register the shutdownNow method of the shutdown executor first. This will cause the shutdown executor
+ // to be closed last and ensure other resources which depend on it can close successfully.
closer.register(exec::shutdownNow);
Review comment:
1. Removed exec::shutdownNow from the closer in the TimedShutoffInputSourceReader#decorateShutdownTimeout. Added a shutdown on the exec instead to make sure that after the timeout period, the exec will automatically be shutdown. This remove the dependency of shutting down the exec from the CloseableIterator#close()
2. Made the recordSuppliers close function thread safe by adding synchronize block. Left the Closers the same as it is also a documented feature/bug on Guava's Closer it is not thread safe (https://github.com/google/guava/issues/3900). We can keep this the same to avoid confusion and possible update our Closer copy when the above issue is done.
The CloseableIterator returned by TimedShutoffInputSourceReader#decorateShutdownTimeout also protect against concurrent close (scheduled exec vs normal closing) by using AtomicBoolean.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r500579681
##########
File path: extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/KinesisRecordSupplier.java
##########
@@ -552,8 +552,11 @@ public void start()
}
}
+ /**
+ * This method is synchronized because multiple threads can attempt to close the record supplier concurrently
+ */
@Override
- public void close()
+ public synchronized void close()
Review comment:
I do think it should be fixed now, ideally, because we can't be confident that this patch actually fixes the bug without fixing the thread safety issue too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r500518971
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/TimedShutoffInputSourceReader.java
##########
@@ -131,6 +134,7 @@ public void close() throws IOException
shutoffTime.getMillis() - System.currentTimeMillis(),
TimeUnit.MILLISECONDS
);
+ exec.shutdown();
Review comment:
This is necessary to shutdown the `exec` created in this class. Previously it was being shutdown via the registering with the Closer. However, that is bad form since the Closer can be executed by a thread in this `exec`. I removed the shutdown of this `exec` from the Closer and added this line. This means that the scheduled task (which calls the iterator.close() method will always run), which is ok since we have the atomic boolean to ensure that the iterator.close() is idempotent. After the task ran then the `exec` will just automatically close. This no longer relies on the Closer, the thread in the `exec` or the iterator.close().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r500474909
##########
File path: extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/KinesisRecordSupplier.java
##########
@@ -552,8 +552,11 @@ public void start()
}
}
+ /**
+ * This method is synchronized because multiple threads can attempt to close the record supplier concurrently
+ */
@Override
- public void close()
+ public synchronized void close()
Review comment:
Same comment as the Kafka version.
##########
File path: extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java
##########
@@ -174,7 +174,7 @@ public Long getPosition(StreamPartition<Integer> partition)
}
@Override
- public void close()
+ public synchronized void close()
Review comment:
This isn't a good way to make the `close` method thread-safe, because it's not guaranteed that it can "see" what other threads are doing. (And it's also not guaranteed that other threads can "see" what the close method does.)
The Kafka consumer itself is also not thread safe, and that'll need to be handled properly.
If the close functionality needs to be thread safe, then the entire class must be adjusted to ensure the logic running in other threads can properly see the close.
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/TimedShutoffInputSourceReader.java
##########
@@ -131,6 +134,7 @@ public void close() throws IOException
shutoffTime.getMillis() - System.currentTimeMillis(),
TimeUnit.MILLISECONDS
);
+ exec.shutdown();
Review comment:
What's the purpose of this line? (It doesn't seem necessary to me)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r499916280
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/TimedShutoffInputSourceReader.java
##########
@@ -68,8 +68,10 @@ public TimedShutoffInputSourceReader(InputSourceReader delegate, DateTime shutof
)
{
final Closer closer = Closer.create();
- closer.register(delegateIterator);
+ // We must register the shutdownNow method of the shutdown executor first. This will cause the shutdown executor
+ // to be closed last and ensure other resources which depend on it can close successfully.
closer.register(exec::shutdownNow);
Review comment:
3. Added tests for the following cases:
- Main thread close the iterator using the close() method (iterator successfully closed before timeout)
- Timeout close the iterator (iterator successfully closed by timeout before the close() of the iterator is called in the main thread)
- Timeout close the iterator and then while it is still closing, main thread close the iterator using the close() method
- Main thread close the iterator using the close() method and then while it is still closing, timeout close the iterator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r499070127
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/TimedShutoffInputSourceReader.java
##########
@@ -68,8 +68,11 @@ public TimedShutoffInputSourceReader(InputSourceReader delegate, DateTime shutof
)
{
final Closer closer = Closer.create();
- closer.register(delegateIterator);
+ // We must register the shutdow of the shutdown executor first. This will make sure that the shutdow of the
Review comment:
nit: typo 'shutdow' -> 'shutdown'.
Also, if you have to change things anyway, that is a lot of shutdown, how about replacing the last 2 sentences with something like "This will cause the executor to be closed last and ensure other resources which might depend on it can close successfully."
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r500521909
##########
File path: extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/KinesisRecordSupplier.java
##########
@@ -552,8 +552,11 @@ public void start()
}
}
+ /**
+ * This method is synchronized because multiple threads can attempt to close the record supplier concurrently
+ */
@Override
- public void close()
+ public synchronized void close()
Review comment:
Maybe it is fine to leave the KinesisRecordSupplier and KafkaRecordSupplier as what it was before (without the synchronized). I think it is sufficient to fix this bug with just the change in TimedShutoffInputSourceReader. The change already make the iterator.close() from the TimedShutoffInputSourceReader idempotent, fix race/concurrent issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] stale[bot] commented on pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10467:
URL: https://github.com/apache/druid/pull/10467#issuecomment-743877599
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r500578893
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/TimedShutoffInputSourceReader.java
##########
@@ -131,6 +134,7 @@ public void close() throws IOException
shutoffTime.getMillis() - System.currentTimeMillis(),
TimeUnit.MILLISECONDS
);
+ exec.shutdown();
Review comment:
I see, so its purpose is to make sure the executor thread is stopped after the task fires. OK, that makes sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10467: Fix bug in TimedShutoffInputSourceReader where a timeout does not close all registered closeable
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r499075337
##########
File path: core/src/main/java/org/apache/druid/data/input/impl/TimedShutoffInputSourceReader.java
##########
@@ -68,8 +68,11 @@ public TimedShutoffInputSourceReader(InputSourceReader delegate, DateTime shutof
)
{
final Closer closer = Closer.create();
- closer.register(delegateIterator);
+ // We must register the shutdow of the shutdown executor first. This will make sure that the shutdow of the
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org