[
https://issues.apache.org/jira/browse/GEODE-8767?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17273954#comment-17273954
]
ASF GitHub Bot commented on GEODE-8767:
---------------------------------------
echobravopapa commented on a change in pull request #5962:
URL: https://github.com/apache/geode/pull/5962#discussion_r566344530
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/direct/DirectChannel.java
##########
@@ -137,7 +142,7 @@ public DirectChannel(Membership<InternalDistributedMember>
mgr,
props.setProperty("membership_port_range_start", "" + range[0]);
props.setProperty("membership_port_range_end", "" + range[1]);
- this.conduit = new TCPConduit(mgr, port, address, isBindAddress, this,
props);
+ this.conduit = new TCPConduit(mgr, port, address, isBindAddress, this,
bufferPool, props);
Review comment:
i.e. providing `bufferPool` directly to `TCPConduit`...
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/direct/DirectChannel.java
##########
@@ -112,6 +115,8 @@ public DirectChannel(Membership<InternalDistributedMember>
mgr,
throws ConnectionException {
this.receiver = listener;
this.dm = dm;
+ this.stats = dm.getStats();
+ this.bufferPool = new BufferPool(stats);
Review comment:
now `DirectChannel` owns `BufferPool` and makes the pool accessible as
needed...
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java
##########
@@ -207,7 +207,7 @@ private ConnectionTable(TCPConduit conduit) {
threadConnectionMap = new ConcurrentHashMap();
p2pReaderThreadPool =
createThreadPoolForIO(conduit.getDM().getSystem().isShareSockets());
socketCloser = new SocketCloser();
- bufferPool = new BufferPool(owner.getStats());
+ bufferPool = conduit.getBufferPool();
Review comment:
`ConnectionTable` formerly owned `BufferPool`
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/direct/DirectChannel.java
##########
@@ -295,7 +307,7 @@ private int sendToMany(final Membership mgr,
List<?> sentCons; // used for cons we sent to this time
final BaseMsgStreamer ms =
- MsgStreamer.create(cons, msg, directReply, stats,
getConduit().getBufferPool());
+ MsgStreamer.create(cons, msg, directReply, stats, bufferPool);
Review comment:
DC owning `bufferPool` looks to make even more sense here
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/tcp/TCPConduitTest.java
##########
@@ -72,11 +73,24 @@ public void setUp() throws Exception {
.thenReturn(mock(DistributionManager.class));
}
+ @Test
+ public void closedConduitDoesNotThrowNPEWhenAskedForBufferPool() {
+ directChannel.getDM(); // Mockito demands that this mock be used in this
test
+ TCPConduit tcpConduit =
+ new TCPConduit(membership, 0, localHost, false, directChannel,
mock(BufferPool.class),
+ new Properties(),
+ TCPConduit -> connectionTable, socketCreator, doNothing(), false);
+ InternalDistributedMember member = mock(InternalDistributedMember.class);
+ tcpConduit.stop(null);
+ assertThat(tcpConduit.getBufferPool()).isNotNull();
Review comment:
and now the `tcpConduit` is constructed with a `BufferPool` this won't
ever be NULL
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java
##########
@@ -207,7 +207,7 @@ private ConnectionTable(TCPConduit conduit) {
threadConnectionMap = new ConcurrentHashMap();
p2pReaderThreadPool =
createThreadPoolForIO(conduit.getDM().getSystem().isShareSockets());
socketCloser = new SocketCloser();
- bufferPool = new BufferPool(owner.getStats());
+ bufferPool = conduit.getBufferPool();
Review comment:
but this was not guaranteed to be available...
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/direct/DirectChannel.java
##########
@@ -112,6 +115,8 @@ public DirectChannel(Membership<InternalDistributedMember>
mgr,
throws ConnectionException {
this.receiver = listener;
this.dm = dm;
+ this.stats = dm.getStats();
+ this.bufferPool = new BufferPool(stats);
Review comment:
and same with `DMStats`
----------------------------------------------------------------
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:
us...@infra.apache.org
> NullPointerException in TCPConduit.getBufferPool due to conTable being null
> on Windows
> --------------------------------------------------------------------------------------
>
> Key: GEODE-8767
> URL: https://issues.apache.org/jira/browse/GEODE-8767
> Project: Geode
> Issue Type: Bug
> Components: membership, messaging
> Affects Versions: 1.14.0
> Reporter: Donal Evans
> Priority: Major
> Labels: pull-request-available
>
> This failure was seen in the WindowsGfshDistributedTestOpenJDK11 CI pipeline
> job:
> {noformat}
> org.apache.geode.management.MemberMXBeanDistributedTest > classMethod FAILED
> java.lang.AssertionError: Suspicious strings were written to the log
> during this run.
> Fix the strings or use IgnoredException.addIgnoredException to ignore.
> -----------------------------------------------------------------------
> Found suspect string in 'dunit_suspect-vm1.log' at line 8350
> [fatal 2020/12/03 20:00:40.000 GMT <Timer-3> tid=630] While pushing
> message <partitioned.PRSanityCheckMessage(prid=3 processorId=0
> regionName=#testCreateRegion2 ,distTx=false)> to recipients:
> <10.0.0.75(server-2:2444)<v2>:41002, 10.0.0.75(server-3:4716)<v3>:41003,
> 10.0.0.75(server-4:6184)<v4>:41004>
> java.lang.NullPointerException
> at
> org.apache.geode.internal.tcp.TCPConduit.getBufferPool(TCPConduit.java:949)
> at
> org.apache.geode.distributed.internal.direct.DirectChannel.sendToMany(DirectChannel.java:298)
> at
> org.apache.geode.distributed.internal.direct.DirectChannel.send(DirectChannel.java:513)
> at
> org.apache.geode.distributed.internal.DistributionImpl.directChannelSend(DistributionImpl.java:346)
> at
> org.apache.geode.distributed.internal.DistributionImpl.send(DistributionImpl.java:291)
> at
> org.apache.geode.distributed.internal.ClusterDistributionManager.sendViaMembershipManager(ClusterDistributionManager.java:2053)
> at
> org.apache.geode.distributed.internal.ClusterDistributionManager.sendOutgoing(ClusterDistributionManager.java:1981)
> at
> org.apache.geode.distributed.internal.ClusterDistributionManager.sendMessage(ClusterDistributionManager.java:2018)
> at
> org.apache.geode.distributed.internal.ClusterDistributionManager.putOutgoing(ClusterDistributionManager.java:1083)
> at
> org.apache.geode.internal.cache.partitioned.PRSanityCheckMessage$1.run2(PRSanityCheckMessage.java:133)
> at
> org.apache.geode.internal.SystemTimer$SystemTimerTask.run(SystemTimer.java:334)
> at java.base/java.util.TimerThread.mainLoop(Timer.java:556)
> at java.base/java.util.TimerThread.run(Timer.java:506)
> 3 tests completed, 1 failed
> {noformat}
> =-=-=-=-=-=-=-=-=-=-=-=-=-=-= Test Results URI
> =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
>
> [http://files.apachegeode-ci.info/builds/apache-develop-main/1.14.0-build.0532/test-results/distributedTest/1607032539/]
>
> =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
> Test report artifacts from this job are available at:
> [http://files.apachegeode-ci.info/builds/apache-develop-main/1.14.0-build.0532/test-artifacts/1607032539/windows-gfshdistributedtest-OpenJDK11-1.14.0-build.0532.tgz]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
