zhoufek commented on pull request #15505: URL: https://github.com/apache/beam/pull/15505#issuecomment-924005236
> I have a few minor comments after looking over the code. But I'm stuck on one high-level question: Is there really no immutable `DirectBuffer` implementation? It seems very odd that we'd need to provide our own. Sorry, I think that the explanation got lost in the design doc among the edits I made in response to feedback. [This page](https://www.javadoc.io/doc/org.agrona/agrona/1.0.5/org/agrona/DirectBuffer.html) gives a list of all known implementations of `DirectBuffer`. Each also implements `MutableDirectBuffer`. [A search](https://github.com/real-logic/agrona/search?q=implements+directbuffer) on Agrona's Github confirms that nothing implements `DirectBuffer` directly. Implementations will either implement `MutableDirectBuffer` or `AtomicBuffer`, the latter of which extends `MutableDirectBuffer`. We could use class with static factory methods that return `DirectBuffer` or `MutableDirectBuffer` as needed, but nothing would stop a user from casting a `DirectBuffer` to its implementation, which will be mutable. We could also write the implementation as a proxy to one of Agrona's implementations, though since this is already mostly just a proxy on top of Java's `ByteBuffer`, we won't gain much (except maybe less testing?). The only thing we'd gain is not having to implement the reading/parsing string methods, but even then, we may want to reimplement some of these methods, for instance: * Parsing doesn't protect against overflow/underflow. While it's still possible to get Long.MAX_VALUE (after double overflow), it can still lead to weird behavior. * Reading a string into an `Appendable` doesn't protect the `Appendable` from partial writes in case of an `IOException`. The interface is set to return the number of bytes written, but at least `UnsafeBuffer` throws an exception ([link](https://github.com/real-logic/agrona/blob/b89428f4dfcffd87fb6faf7e77f62fbc7d48b9c4/agrona/src/main/java/org/agrona/concurrent/UnsafeBuffer.java#L1343)). Overall, it just seemed the best decision was to write our own implementation on top of `ByteBuffer`. It keeps the overall logic on our end small while giving us better control over the string methods. -- 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]
