[GitHub] [geode] Bill commented on a change in pull request #5562: GEODE-8542: java.lang.IllegalStateException: tcp message exceeded max…

[GitBox <gi...@apache.org>]

Bill commented on a change in pull request #5562:
URL: https://github.com/apache/geode/pull/5562#discussion_r496068316



##########
File path: geode-core/src/test/java/org/apache/geode/internal/tcp/MsgStreamerTest.java
##########
@@ -92,9 +117,9 @@ protected BaseMsgStreamer createMsgStreamer(boolean mixedDestinationVersions) {
     when(connection2.getRemoteAddress()).thenReturn(member2);
     when(connection2.getSendBufferSize()).thenReturn(Connection.SMALL_BUFFER_SIZE);
     if (mixedDestinationVersions) {
-      when(connection1.getRemoteVersion()).thenReturn(KnownVersion.GEODE_1_12_0);
+      when(connection2.getRemoteVersion()).thenReturn(KnownVersion.GEODE_1_12_0);
     } else {
-      when(connection1.getRemoteVersion()).thenReturn(KnownVersion.CURRENT);
+      when(connection2.getRemoteVersion()).thenReturn(KnownVersion.CURRENT);

Review comment:
       I don't know why this code was changed to make the second connection (instead of the first), the old version connection. But the resulting code seems just as correct as the old code  ✓

##########
File path: geode-core/src/main/java/org/apache/geode/internal/tcp/MsgStreamer.java
##########
@@ -129,7 +130,8 @@ public ConnectExceptions getConnectExceptions() {
     this.stats = stats;
     this.msg = msg;
     this.cons = cons;
-    this.buffer = bufferPool.acquireDirectSenderBuffer(sendBufferSize);
+    int bufferSize = Math.min(sendBufferSize, Connection.MAX_MSG_SIZE);
+    this.buffer = bufferPool.acquireDirectSenderBuffer(bufferSize);

Review comment:
       since this class is in the same package as `Connection` this seems like appropriate coupling ✓

##########
File path: geode-core/src/test/java/org/apache/geode/internal/tcp/MsgStreamerTest.java
##########
@@ -77,6 +77,31 @@ public void streamerListReleaseWithException() throws IOException {
     verify(pool, times(2)).releaseSenderBuffer(isA(ByteBuffer.class));
   }
 
+  @Test
+  public void streamerRespectsMaxMessageSize() {
+    InternalDistributedMember member1;
+    member1 = new InternalDistributedMember("localhost", 1234);
+
+    DistributionMessage message = new SerialAckedMessage();
+    message.setRecipients(Arrays.asList(member1));
+
+    when(connection1.getRemoteAddress()).thenReturn(member1);
+    when(connection1.getRemoteVersion()).thenReturn(KnownVersion.CURRENT);
+    // create a streamer for a Connection that has a buffer size that's larger than the
+    // biggest message we can actually send. This is picked up by the MsgStreamer to allocate
+    // a buffer
+    when(connection1.getSendBufferSize()).thenReturn(Connection.MAX_MSG_SIZE * 2);

Review comment:
       This test could be stronger if the requested message size was `MAX_MSG_SIZE + 1`. As it stands this test allows a lot of wiggle-room for the implementation.




----------------------------------------------------------------
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:
users@infra.apache.org