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/05/04 21:34:35 UTC
[GitHub] [geode] bschuchardt opened a new pull request #5048: GEODE-8020: buffer management problems
bschuchardt opened a new pull request #5048:
URL: https://github.com/apache/geode/pull/5048
This fixes some buffer handling in MsgStreamerList and alters
MstStreamer to avoid creating MsgStreamerList and VersionedMsgStreamers
during normal, non-upgrade, operations.
It also changes NioSslEngine to use synchronization in more places,
notably the close() method, which was possibly allowing multiple threads to
change the state of the engine.
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 #5048: GEODE-8020: buffer management problems
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #5048:
URL: https://github.com/apache/geode/pull/5048#discussion_r420837745
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java
##########
@@ -398,6 +398,7 @@ public void close(SocketChannel socketChannel) {
} finally {
bufferPool.releaseBuffer(TRACKED_SENDER, myNetData);
bufferPool.releaseBuffer(TRACKED_RECEIVER, peerAppData);
+ myNetData = null;
Review comment:
that's cute, personalized data ;P
##########
File path: geode-core/src/main/java/org/apache/geode/internal/tcp/MsgStreamerList.java
##########
@@ -63,25 +63,16 @@ public int writeMessage() throws IOException {
for (MsgStreamer streamer : this.streamers) {
if (ex != null) {
streamer.release();
- // TODO: shouldn't we call continue here?
- // It seems wrong to call writeMessage on a streamer we have just released.
- // But why do we call release on a streamer when we had an exception on one
- // of the previous streamer?
- // release clears the direct bb and returns it to the pool but leaves
- // it has the "buffer". THen we call writeMessage and it will use "buffer"
- // that has also been returned to the pool.
- // I think we only have a MsgStreamerList when a DS has a mix of versions
- // which usually is just during a rolling upgrade so that might be why we
- // haven't noticed this causing a bug.
- }
- try {
- result += streamer.writeMessage();
- // if there is an exception we need to finish the
- // loop and release the other streamer's buffers
- } catch (RuntimeException e) {
- ex = e;
- } catch (IOException e) {
- ioex = e;
+ } else {
Review comment:
thx for removing all those crufty comments
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -69,7 +74,8 @@ public BufferPool(DMStats stats) {
/**
* use direct ByteBuffers instead of heap ByteBuffers for NIO operations
*/
- public static final boolean useDirectBuffers = !Boolean.getBoolean("p2p.nodirectBuffers");
+ public static final boolean useDirectBuffers = !(Boolean.getBoolean("p2p.nodirectBuffers")
+ || Boolean.getBoolean(GeodeGlossary.GEMFIRE_PREFIX + "noDirectBuffers"));
Review comment:
is the GF prefixed version of the property need to be exposed/documented?
----------------------------------------------------------------
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] pivotal-jbarrett commented on pull request #5048: GEODE-8020: buffer management problems
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #5048:
URL: https://github.com/apache/geode/pull/5048#issuecomment-624710775
Can we make the new geode property positive by dropping the no and flipping the default.
----------------------------------------------------------------
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 pull request #5048: GEODE-8020: buffer management problems
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on pull request #5048:
URL: https://github.com/apache/geode/pull/5048#issuecomment-624719148
> Can we make the new geode property positive by dropping the no and flipping the default.
Good point, and the new property hints that nothing will use direct-buffers. How about this?
GeodeGlossary.GEMFIRE_PREFIX + "BufferPool.useHeapBuffers"
----------------------------------------------------------------
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 #5048: GEODE-8020: buffer management problems
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #5048:
URL: https://github.com/apache/geode/pull/5048#discussion_r420853255
##########
File path: geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java
##########
@@ -69,7 +74,8 @@ public BufferPool(DMStats stats) {
/**
* use direct ByteBuffers instead of heap ByteBuffers for NIO operations
*/
- public static final boolean useDirectBuffers = !Boolean.getBoolean("p2p.nodirectBuffers");
+ public static final boolean useDirectBuffers = !(Boolean.getBoolean("p2p.nodirectBuffers")
+ || Boolean.getBoolean(GeodeGlossary.GEMFIRE_PREFIX + "noDirectBuffers"));
Review comment:
> is the GF prefixed version of the property need to be exposed/documented?
yes, but I'm not sure what the best way is to do that. I updated properties.html in this PR with the new property.
----------------------------------------------------------------
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