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 2021/11/19 19:14:52 UTC
[GitHub] [geode] Bill opened a new pull request #7131: GEODE-9825: Reader Retains Unprocessed Bytes
Bill opened a new pull request #7131:
URL: https://github.com/apache/geode/pull/7131
[GEODE-9825](https://issues.apache.org/jira/browse/GEODE-9825)
When `Connection.processInputBuffer()` needed to expand the input buffer, it was also destroying unprocessed data. This resulted in `IOException: "Unknown header byte..."` and hangs.
The `P2PMessagingConcurrencyDUnitTest` has been enhanced to test a matrix of configurations:
[conserve-sockets, useTLS, socket-buffer-size for sender, socket-buffer-size for receiver]
true, true, 32768, 32768
true, true, 65536, 32768
true, false, 32768, 32768
true, false, 65536, 32768
false, true, 32768, 32768
false, true, 65536, 32768
false, false, 32768, 32768
false, false, 65536, 32768
Without the fix in this PR, these were the test results:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #7131: GEODE-9825: Disparate socket-buffer-size Results in "IOException: Unknown header byte" and Hangs
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #7131:
URL: https://github.com/apache/geode/pull/7131#discussion_r753465082
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/distributed/internal/P2PMessagingConcurrencyDUnitTest.java
##########
@@ -51,12 +53,20 @@
import org.apache.geode.test.version.VersionManager;
/**
- * Tests one-way P2P messaging between two peers. A shared,
- * ordered connection is used and many concurrent tasks
- * compete on the sending side. Tests with TLS enabled
- * to exercise ByteBufferSharing and friends.
+ * Tests one-way P2P messaging between two peers.
+ * Many concurrent tasks compete on the sending side.
+ * The main purpose of the test is to exercise
+ * ByteBufferSharing and friends.
+ *
+ * Tests combinations of: conserve-sockets true/false,
+ * TLS on/off, and socket-buffer-size for sender
+ * and receiver both set to the default (and equal)
+ * and set to the sender's buffer twice as big as the
+ * receiver's buffer.
+ *
*/
@Category({MembershipTest.class})
+@RunWith(JUnitParamsRunner.class)
Review comment:
You need the category params runner or delete the category, without that runner the category is ignored.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java
##########
@@ -2846,6 +2846,9 @@ private void processInputBuffer(AbstractExecutor threadMonitorExecutor)
"Allocating larger network read buffer, new size is {} old size was {}.",
allocSize, oldBufferSize);
inputBuffer = inputSharing.expandReadBufferIfNeeded(allocSize);
+ // we're returning to the caller (done == true) so make buffer writable
Review comment:
If we have to comment it to make it readable perhaps it deserves a well named method.
```java
...
makeWriteable(inputBuffer);
...
void makeWriteable(final ByteByffer buffer) {
buffer.position(buffer.limit());
buffer.limit(buffer.capacity());
}
```
Now this method is unit testable too.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #7131: GEODE-9825: Disparate socket-buffer-size Results in "IOException: Unknown header byte" and Hangs
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #7131:
URL: https://github.com/apache/geode/pull/7131#discussion_r753524441
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/distributed/internal/P2PMessagingConcurrencyDUnitTest.java
##########
@@ -87,21 +99,44 @@
*/
private static LongAdder bytesTransferredAdder;
- @Before
- public void before() throws GeneralSecurityException, IOException {
- final Properties configuration = gemFireConfiguration();
+ private void configure(
+ final boolean conserveSockets,
+ final boolean useTLS,
+ final int sendSocketBufferSize,
+ final int receiveSocketBufferSize) throws GeneralSecurityException, IOException {
+
+ final Properties senderConfiguration =
+ gemFireConfiguration(conserveSockets, useTLS, sendSocketBufferSize);
+ final Properties receiverConfiguration =
+ gemFireConfiguration(conserveSockets, useTLS, receiveSocketBufferSize);
final MemberVM locator =
clusterStartupRule.startLocatorVM(0, 0, VersionManager.CURRENT_VERSION,
- x -> x.withProperties(configuration).withConnectionToLocator()
+ x -> x.withProperties(senderConfiguration).withConnectionToLocator()
.withoutClusterConfigurationService().withoutManagementRestService());
- sender = clusterStartupRule.startServerVM(1, configuration, locator.getPort());
- receiver = clusterStartupRule.startServerVM(2, configuration, locator.getPort());
+ sender = clusterStartupRule.startServerVM(1, senderConfiguration, locator.getPort());
+ receiver = clusterStartupRule.startServerVM(2, receiverConfiguration, locator.getPort());
}
@Test
- public void testP2PMessagingWithTLS() {
+ @Parameters({
+ "true, true, 32768, 32768",
+ "true, true, 65536, 32768",
Review comment:
Its good to have coverage for the case where sender is small and receiver is large; to test any changes in future.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #7131: GEODE-9825: Disparate socket-buffer-size Results in "IOException: Unknown header byte" and Hangs
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #7131:
URL: https://github.com/apache/geode/pull/7131#discussion_r754423710
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/distributed/internal/P2PMessagingConcurrencyDUnitTest.java
##########
@@ -87,21 +99,44 @@
*/
private static LongAdder bytesTransferredAdder;
- @Before
- public void before() throws GeneralSecurityException, IOException {
- final Properties configuration = gemFireConfiguration();
+ private void configure(
+ final boolean conserveSockets,
+ final boolean useTLS,
+ final int sendSocketBufferSize,
+ final int receiveSocketBufferSize) throws GeneralSecurityException, IOException {
+
+ final Properties senderConfiguration =
+ gemFireConfiguration(conserveSockets, useTLS, sendSocketBufferSize);
+ final Properties receiverConfiguration =
+ gemFireConfiguration(conserveSockets, useTLS, receiveSocketBufferSize);
final MemberVM locator =
clusterStartupRule.startLocatorVM(0, 0, VersionManager.CURRENT_VERSION,
- x -> x.withProperties(configuration).withConnectionToLocator()
+ x -> x.withProperties(senderConfiguration).withConnectionToLocator()
.withoutClusterConfigurationService().withoutManagementRestService());
- sender = clusterStartupRule.startServerVM(1, configuration, locator.getPort());
- receiver = clusterStartupRule.startServerVM(2, configuration, locator.getPort());
+ sender = clusterStartupRule.startServerVM(1, senderConfiguration, locator.getPort());
+ receiver = clusterStartupRule.startServerVM(2, receiverConfiguration, locator.getPort());
}
@Test
- public void testP2PMessagingWithTLS() {
+ @Parameters({
+ "true, true, 32768, 32768",
+ "true, true, 65536, 32768",
Review comment:
done
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill merged pull request #7131: GEODE-9825: Disparate socket-buffer-size Results in "IOException: Unknown header byte" and Hangs
Posted by GitBox <gi...@apache.org>.
Bill merged pull request #7131:
URL: https://github.com/apache/geode/pull/7131
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on pull request #7131: GEODE-9825: Disparate socket-buffer-size Results in "IOException: Unknown header byte" and Hangs
Posted by GitBox <gi...@apache.org>.
Bill commented on pull request #7131:
URL: https://github.com/apache/geode/pull/7131#issuecomment-974456704
Ooh the stress test found a failure!
This fix may be insufficient.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org