Copilot commented on code in PR #19827:
URL: https://github.com/apache/druid/pull/19827#discussion_r3685670896
##########
extensions-core/lookups-cached-global/src/main/java/org/apache/druid/server/lookup/namespace/cache/OffHeapNamespaceExtractionCacheManager.java:
##########
@@ -192,12 +198,14 @@ public void start()
}
@Override
- public synchronized void stop()
+ public void stop()
{
- if (!mmapDB.isClosed()) {
- mmapDB.close();
- if (!tmpFile.delete()) {
- log.warn("Unable to delete file at [%s]",
tmpFile.getAbsolutePath());
+ synchronized (mmapDB) {
+ if (!mmapDB.isClosed()) {
+ mmapDB.close();
+ if (!tmpFile.delete()) {
+ log.warn("Unable to delete file at [%s]",
tmpFile.getAbsolutePath());
Review Comment:
`stop()` holds the `mmapDB` monitor while deleting `tmpFile` and potentially
logging. Since MapDB state-changing APIs synchronize on the `DB` instance,
doing filesystem I/O under this lock can unnecessarily block other MapDB
callers and extend shutdown latency. Consider limiting the synchronized section
to just the `isClosed()`/`close()` check-use sequence, then delete/log after
releasing the monitor.
##########
processing/src/main/java/org/apache/druid/java/util/http/client/io/AppendableByteArrayInputStream.java:
##########
@@ -51,15 +51,15 @@ public void add(byte[] bytesToAdd)
synchronized (singleByteReaderDoer) {
bytes.addLast(bytesToAdd);
available += bytesToAdd.length;
- singleByteReaderDoer.notify();
+ singleByteReaderDoer.notifyAll();
}
Review Comment:
`add()` now uses `notifyAll()`, which can allow multiple threads blocked in
`read()` to wake and proceed concurrently. This class mutates shared
`curr`/`currIndex` outside the monitor, so concurrent reads can corrupt stream
state (duplicate/skip bytes, negative `available`, etc.). If multiple readers
are a real scenario, the read path needs serialization (e.g., guard
`curr`/`currIndex` updates under a lock) or the class should enforce/document a
single-reader contract and avoid waking multiple readers on data addition.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
