jtuglu-netflix commented on code in PR #17652:
URL: https://github.com/apache/druid/pull/17652#discussion_r1925885503
##########
processing/src/main/java/org/apache/druid/io/LimitedOutputStream.java:
##########
@@ -24,18 +24,19 @@
import java.io.IOException;
import java.io.OutputStream;
+import java.util.concurrent.atomic.AtomicLong;
import java.util.function.Function;
/**
* An {@link OutputStream} that limits how many bytes can be written. Throws
{@link IOException} if the limit
- * is exceeded.
+ * is exceeded. *Not* thread-safe.
*/
public class LimitedOutputStream extends OutputStream
{
private final OutputStream out;
private final long limit;
private final Function<Long, String> exceptionMessageFn;
- long written;
+ AtomicLong written;
Review Comment:
I get your point, That `AtomicLong` effectively just ensures the *worst*
case doesn't happen if someone uses it incorrectly (a race that causes torn
writes (2 threads write a combined N bytes) which succeed to a buffer with (<N)
byte to spare). I can remove, but until a properly thread-safe solution for a
byte-limited version of `ByteArrayOutputStream` is made, I figured I'd keep it.
This will involve more changes not related to this bug-fix which I think be
logically separated into another PR. It's effectively defensive programming
against the worst-case race if used improperly (multi-threaded setting). I can
switch back, but I don't see the harm in keeping it.
--
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]
