You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/09/25 22:57:50 UTC
[GitHub] [geode] bschuchardt opened a new pull request #5562: GEODE-8542: java.lang.IllegalStateException: tcp message exceeded max…
bschuchardt opened a new pull request #5562:
URL: https://github.com/apache/geode/pull/5562
… size of 16777215
Limit the size of message chunks to the maximum message size allowed
by org.apache.geode.internal.tcp.Connection.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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
[GitHub] [geode] echobravopapa commented on a change in pull request #5562: GEODE-8542: java.lang.IllegalStateException: tcp message exceeded max…
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #5562:
URL: https://github.com/apache/geode/pull/5562#discussion_r496195999
##########
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:
`connection2` does not seem to be used in the test cases
----------------------------------------------------------------
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
[GitHub] [geode] bschuchardt commented on a change in pull request #5562: GEODE-8542: java.lang.IllegalStateException: tcp message exceeded max…
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5562:
URL: https://github.com/apache/geode/pull/5562#discussion_r496778216
##########
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:
@Bill the version for connection1 was already established. The if/else statement is to establish the version for connection2. This was a cut and paste error that I fixed.
----------------------------------------------------------------
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
[GitHub] [geode] Bill commented on a change in pull request #5562: GEODE-8542: java.lang.IllegalStateException: tcp message exceeded max…
Posted by 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
[GitHub] [geode] bschuchardt merged pull request #5562: GEODE-8542: java.lang.IllegalStateException: tcp message exceeded max…
Posted by GitBox <gi...@apache.org>.
bschuchardt merged pull request #5562:
URL: https://github.com/apache/geode/pull/5562
----------------------------------------------------------------
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