You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Kirk Lund <kl...@apache.org> on 2018/06/04 16:50:17 UTC
PR #1868: new ThreadMonitoring feature
Can a couple more folks please review PR #1868?
My review feedback pertained only to using dependency injection instead of
a singleton or static getter and Yossi has updated the code as I requested.
My review doesn't really address the usefulness of the feature or the
impact on features that were updated to use ThreadMonitoring.
Note that there is new User API and Configuration.
It would be good for people who work with Queues, Pools, Configuration,
FunctionExecutor, DistributionManager (and its Executors), Client/Server
communication, WAN Gateways to review this PR since it touches all of these
classes:
AbstractGatewaySenderEventProcessor.java
AcceptorImpl.java
ClusterDistributionManager.java
ConcurrentParallelGatewaySenderEventProcessor.java
ConcurrentSerialGatewaySenderEventProcessor.java
DistributionConfig.java
DistributionConfigImpl.java
DistributionManager.java
ExpiryTask.java
FunctionExecutionPooledExecutor.java
GemFireCacheImpl.java
LonerDistributionManager.java
OneTaskOnlyExecutor.java
ParallelAsyncEventQueueImpl.java
ParallelGatewaySenderEventProcessor.java
ParallelGatewaySenderHelper.java
ParallelGatewaySenderImpl.java
PoolImpl.java
PRHARedundancyProvider.java
RemoteConcurrentParallelGatewaySenderEventProcessor.java
RemoteConcurrentSerialGatewaySenderEventProcessor.java
RemoteParallelGatewaySenderEventProcessor.java
RemoteSerialGatewaySenderEventProcessor.java
ScheduledThreadPoolExecutorWithKeepAlive.java
SerialAsyncEventQueueImpl.java
SerialGatewaySenderEventProcessor.java
SerialGatewaySenderImpl.java
SerialQueuedExecutorWithDMStats.java
TcpServer.java
This PR also introduces new user visible API and configuration options:
ConfigurationProperties.java
ThreadMonitoring.java
(I may have left out a couple classes)
https://github.com/apache/geode/pull/1868
Re: PR #1868: new ThreadMonitoring feature
Posted by Bruce Schuchardt <bs...@pivotal.io>.
This seems like a really bad idea. It's using the long-deprecated
Thread.stop() to kill executor threads that take too long to execute.
It has no idea what state the thread is in when it kills it. It could
be doing disk I/O or writing to shared memory.
On 6/4/18 9:50 AM, Kirk Lund wrote:
> Can a couple more folks please review PR #1868?
>
> My review feedback pertained only to using dependency injection instead of
> a singleton or static getter and Yossi has updated the code as I requested.
> My review doesn't really address the usefulness of the feature or the
> impact on features that were updated to use ThreadMonitoring.
>
> Note that there is new User API and Configuration.
>
> It would be good for people who work with Queues, Pools, Configuration,
> FunctionExecutor, DistributionManager (and its Executors), Client/Server
> communication, WAN Gateways to review this PR since it touches all of these
> classes:
>
> AbstractGatewaySenderEventProcessor.java
> AcceptorImpl.java
> ClusterDistributionManager.java
> ConcurrentParallelGatewaySenderEventProcessor.java
> ConcurrentSerialGatewaySenderEventProcessor.java
> DistributionConfig.java
> DistributionConfigImpl.java
> DistributionManager.java
> ExpiryTask.java
> FunctionExecutionPooledExecutor.java
> GemFireCacheImpl.java
> LonerDistributionManager.java
> OneTaskOnlyExecutor.java
> ParallelAsyncEventQueueImpl.java
> ParallelGatewaySenderEventProcessor.java
> ParallelGatewaySenderHelper.java
> ParallelGatewaySenderImpl.java
> PoolImpl.java
> PRHARedundancyProvider.java
> RemoteConcurrentParallelGatewaySenderEventProcessor.java
> RemoteConcurrentSerialGatewaySenderEventProcessor.java
> RemoteParallelGatewaySenderEventProcessor.java
> RemoteSerialGatewaySenderEventProcessor.java
> ScheduledThreadPoolExecutorWithKeepAlive.java
> SerialAsyncEventQueueImpl.java
> SerialGatewaySenderEventProcessor.java
> SerialGatewaySenderImpl.java
> SerialQueuedExecutorWithDMStats.java
> TcpServer.java
>
> This PR also introduces new user visible API and configuration options:
>
> ConfigurationProperties.java
> ThreadMonitoring.java
>
> (I may have left out a couple classes)
>
> https://github.com/apache/geode/pull/1868
>
Re: PR #1868: new ThreadMonitoring feature
Posted by Udo Kohlmeyer <ud...@apache.org>.
Some great points Bruce!!
I believe this whole thread Monitoring feature needs to have the scope
expanded... I mean.. We would either require compensatory events to
rollback ANYTHING that the thread had been doing until the point it was
killed.
When it was proposed, I understood it as a mechanism that would let us
know if there are threads that are stuck, in real time.... Seems this PR
has some remediation build it, which would require A LOT more thought!!!
--Udo
On 6/4/18 13:55, Bruce Schuchardt wrote:
> This looks like a dangerous change that is putting a fixed time limit
> on execution of arbitrary jobs with no real check to see if threads
> are stuck or not. If a thread is doing some heavy lifting it could be
> killed with Thread.stop() in the middle of its work. Think initial
> image transfer, for instance. This can leave "sender" threads stuck
> waiting for replies until they, too, are killed.
>
> No-one is even supposed to /use/ Thread.stop().
>
>
> On 6/4/18 9:50 AM, Kirk Lund wrote:
>> Can a couple more folks please review PR #1868?
>>
>> My review feedback pertained only to using dependency injection
>> instead of
>> a singleton or static getter and Yossi has updated the code as I
>> requested.
>> My review doesn't really address the usefulness of the feature or the
>> impact on features that were updated to use ThreadMonitoring.
>>
>> Note that there is new User API and Configuration.
>>
>> It would be good for people who work with Queues, Pools, Configuration,
>> FunctionExecutor, DistributionManager (and its Executors), Client/Server
>> communication, WAN Gateways to review this PR since it touches all of
>> these
>> classes:
>>
>> AbstractGatewaySenderEventProcessor.java
>> AcceptorImpl.java
>> ClusterDistributionManager.java
>> ConcurrentParallelGatewaySenderEventProcessor.java
>> ConcurrentSerialGatewaySenderEventProcessor.java
>> DistributionConfig.java
>> DistributionConfigImpl.java
>> DistributionManager.java
>> ExpiryTask.java
>> FunctionExecutionPooledExecutor.java
>> GemFireCacheImpl.java
>> LonerDistributionManager.java
>> OneTaskOnlyExecutor.java
>> ParallelAsyncEventQueueImpl.java
>> ParallelGatewaySenderEventProcessor.java
>> ParallelGatewaySenderHelper.java
>> ParallelGatewaySenderImpl.java
>> PoolImpl.java
>> PRHARedundancyProvider.java
>> RemoteConcurrentParallelGatewaySenderEventProcessor.java
>> RemoteConcurrentSerialGatewaySenderEventProcessor.java
>> RemoteParallelGatewaySenderEventProcessor.java
>> RemoteSerialGatewaySenderEventProcessor.java
>> ScheduledThreadPoolExecutorWithKeepAlive.java
>> SerialAsyncEventQueueImpl.java
>> SerialGatewaySenderEventProcessor.java
>> SerialGatewaySenderImpl.java
>> SerialQueuedExecutorWithDMStats.java
>> TcpServer.java
>>
>> This PR also introduces new user visible API and configuration options:
>>
>> ConfigurationProperties.java
>> ThreadMonitoring.java
>>
>> (I may have left out a couple classes)
>>
>> https://github.com/apache/geode/pull/1868
>>
>
>
Re: PR #1868: new ThreadMonitoring feature
Posted by Bruce Schuchardt <bs...@pivotal.io>.
This looks like a dangerous change that is putting a fixed time limit on
execution of arbitrary jobs with no real check to see if threads are
stuck or not. If a thread is doing some heavy lifting it could be
killed with Thread.stop() in the middle of its work. Think initial image
transfer, for instance. This can leave "sender" threads stuck waiting
for replies until they, too, are killed.
No-one is even supposed to /use/ Thread.stop().
On 6/4/18 9:50 AM, Kirk Lund wrote:
> Can a couple more folks please review PR #1868?
>
> My review feedback pertained only to using dependency injection instead of
> a singleton or static getter and Yossi has updated the code as I requested.
> My review doesn't really address the usefulness of the feature or the
> impact on features that were updated to use ThreadMonitoring.
>
> Note that there is new User API and Configuration.
>
> It would be good for people who work with Queues, Pools, Configuration,
> FunctionExecutor, DistributionManager (and its Executors), Client/Server
> communication, WAN Gateways to review this PR since it touches all of these
> classes:
>
> AbstractGatewaySenderEventProcessor.java
> AcceptorImpl.java
> ClusterDistributionManager.java
> ConcurrentParallelGatewaySenderEventProcessor.java
> ConcurrentSerialGatewaySenderEventProcessor.java
> DistributionConfig.java
> DistributionConfigImpl.java
> DistributionManager.java
> ExpiryTask.java
> FunctionExecutionPooledExecutor.java
> GemFireCacheImpl.java
> LonerDistributionManager.java
> OneTaskOnlyExecutor.java
> ParallelAsyncEventQueueImpl.java
> ParallelGatewaySenderEventProcessor.java
> ParallelGatewaySenderHelper.java
> ParallelGatewaySenderImpl.java
> PoolImpl.java
> PRHARedundancyProvider.java
> RemoteConcurrentParallelGatewaySenderEventProcessor.java
> RemoteConcurrentSerialGatewaySenderEventProcessor.java
> RemoteParallelGatewaySenderEventProcessor.java
> RemoteSerialGatewaySenderEventProcessor.java
> ScheduledThreadPoolExecutorWithKeepAlive.java
> SerialAsyncEventQueueImpl.java
> SerialGatewaySenderEventProcessor.java
> SerialGatewaySenderImpl.java
> SerialQueuedExecutorWithDMStats.java
> TcpServer.java
>
> This PR also introduces new user visible API and configuration options:
>
> ConfigurationProperties.java
> ThreadMonitoring.java
>
> (I may have left out a couple classes)
>
> https://github.com/apache/geode/pull/1868
>