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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
