Bill commented on a change in pull request #5363:
URL: https://github.com/apache/geode/pull/5363#discussion_r454741200
##########
File path: geode-core/src/main/java/org/apache/geode/internal/tcp/MsgReader.java
##########
@@ -14,70 +14,65 @@
*/
package org.apache.geode.internal.tcp;
+import java.io.EOFException;
import java.io.IOException;
-import java.nio.BufferUnderflowException;
+import java.io.InputStream;
+import java.net.Socket;
import java.nio.ByteBuffer;
-import org.apache.logging.log4j.Logger;
-
import org.apache.geode.distributed.internal.DMStats;
import org.apache.geode.distributed.internal.DistributionMessage;
import org.apache.geode.distributed.internal.ReplyProcessor21;
import org.apache.geode.internal.Assert;
import org.apache.geode.internal.InternalDataSerializer;
import org.apache.geode.internal.net.BufferPool;
-import org.apache.geode.internal.net.NioFilter;
+import org.apache.geode.internal.net.SocketUtils;
import org.apache.geode.internal.serialization.Version;
-import org.apache.geode.logging.internal.log4j.api.LogService;
/**
* This class is currently used for reading direct ack responses It should
probably be used for all
* of the reading done in Connection.
*
*/
public class MsgReader {
- private static final Logger logger = LogService.getLogger();
-
- protected final Connection conn;
+ protected final ClusterConnection conn;
+ private final BufferPool bufferPool;
protected final Header header = new Header();
- private final NioFilter ioFilter;
private ByteBuffer peerNetData;
+ private final InputStream inputStream;
private final ByteBufferInputStream byteBufferInputStream;
+ private int lastProcessedPosition;
+ private int lastReadPosition;
Review comment:
My comment came out of frustration, perhaps born of laziness. I'm trying
to decide whether or not I think the new code in `MsgReader` is correct—by
inspection. That's very hard for me.
Earlier in the day I went and read the NIO `Buffer` code since I needed to
understand that code to understand these changes. That class has [Jon
Bentley](https://www.goodreads.com/book/show/52084.Programming_Pearls)-level
comments detailing the purpose of each field along with the field invariants.
The documentation of those invariants are a huge aid in visual inspection.
https://docs.oracle.com/javase/8/docs/api/java/nio/Buffer.html
So with this in mind:
1. my hunch is that `Buffer` already has all the state we need to solve this
problem—so I am suspicious of these added fields
2. it feels like we're doing a lot of poking into the `Buffer` object to
control its details—setting limit and position explicitly rather than putting
and getting and flipping
If the extra fields are really needed then it would help to see a block
comment at the point of declaration, tying together these variables with their
invariants. Rather than saying things like "keep track of", it would be helpful
to know the relationships between these two variables and between them and the
various state variables inside the `Buffer`.
----------------------------------------------------------------
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]
