Details

    Description

      "vm_9_thr_27_persist8_host1_29953" #1448 daemon prio=5 os_prio=0 cpu=45.99ms elapsed=698.42s tid=0x00007f8d0c052800 nid=0x2ee8 waiting on condition [0x00007f8c354f6000]
      java.lang.Thread.State: TIMED_WAITING (parking)
      at jdk.internal.misc.Unsafe.park(java.base@11.0.9/Native Method)

      • parking to wait for <0x00000000fb7bb488> (a java.util.concurrent.CountDownLatch$Sync)
        at java.util.concurrent.locks.LockSupport.parkNanos(java.base@11.0.9/LockSupport.java:234)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedNanos(java.base@11.0.9/AbstractQueuedSynchronizer.java:1079)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.tryAcquireSharedNanos(java.base@11.0.9/AbstractQueuedSynchronizer.java:1369)
        at java.util.concurrent.CountDownLatch.await(java.base@11.0.9/CountDownLatch.java:278)
        at org.apache.geode.internal.util.concurrent.StoppableCountDownLatch.await(StoppableCountDownLatch.java:72)
        at org.apache.geode.distributed.internal.ReplyProcessor21.basicWait(ReplyProcessor21.java:723)
        at org.apache.geode.distributed.internal.ReplyProcessor21.waitForRepliesUninterruptibly(ReplyProcessor21.java:794)
        at org.apache.geode.distributed.internal.ReplyProcessor21.waitForRepliesUninterruptibly(ReplyProcessor21.java:771)
        at org.apache.geode.distributed.internal.ReplyProcessor21.waitForRepliesUninterruptibly(ReplyProcessor21.java:857)
        at org.apache.geode.distributed.internal.locks.DLockRecoverGrantorProcessor.recoverLockGrantor(DLockRecoverGrantorProcessor.java:100)
        at org.apache.geode.distributed.internal.locks.DLockService.makeLocalGrantor(DLockService.java:447)
        at org.apache.geode.distributed.internal.locks.DLockService.createLocalGrantor(DLockService.java:392)
        at org.apache.geode.distributed.internal.locks.DLockService.getLockGrantorId(DLockService.java:337)
        at org.apache.geode.distributed.internal.locks.DLockService.lockInterruptibly(DLockService.java:1445)
        at org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1241)
        at org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1232)
        at org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1227)
        at org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1223)
        at org.apache.geode.pdx.internal.PeerTypeRegistration.lock(PeerTypeRegistration.java:314)
        at org.apache.geode.pdx.internal.PeerTypeRegistration.defineEnum(PeerTypeRegistration.java:646)
        at org.apache.geode.pdx.internal.PeerTypeRegistration.getEnumId(PeerTypeRegistration.java:601)
        at org.apache.geode.pdx.internal.TypeRegistry.getEnumId(TypeRegistry.java:363)
        at org.apache.geode.internal.InternalDataSerializer.writePdxEnum(InternalDataSerializer.java:2071)
        at org.apache.geode.internal.InternalDataSerializer.writeUserObject(InternalDataSerializer.java:1610)
        at org.apache.geode.internal.InternalDataSerializer.writeWellKnownObject(InternalDataSerializer.java:1517)
        at org.apache.geode.internal.InternalDataSerializer.basicWriteObject(InternalDataSerializer.java:2034)
        at org.apache.geode.pdx.internal.PdxOutputStream.writeObject(PdxOutputStream.java:72)
        at org.apache.geode.pdx.internal.PdxWriterImpl.writeObject(PdxWriterImpl.java:341)
        at org.apache.geode.pdx.internal.PdxWriterImpl.writeObject(PdxWriterImpl.java:330)
        at util.VersionedValueHolder.myToData(VersionedValueHolder.java:227)
        at util.PdxVersionedValueHolder.toData(PdxVersionedValueHolder.java:84)
        at org.apache.geode.internal.InternalDataSerializer.writePdx(InternalDataSerializer.java:2794)
        at org.apache.geode.internal.InternalDataSerializer.basicWriteObject(InternalDataSerializer.java:2011)
        at org.apache.geode.DataSerializer.writeObject(DataSerializer.java:2839)
        at org.apache.geode.internal.util.BlobHelper.serializeToBlob(BlobHelper.java:54)
        at org.apache.geode.internal.cache.EntryEventImpl.serialize(EntryEventImpl.java:2092)
        at org.apache.geode.internal.cache.EntryEventImpl.serialize(EntryEventImpl.java:2078)
        at org.apache.geode.internal.cache.entries.DiskEntry$Helper.createValueWrapper(DiskEntry.java:768)
        at org.apache.geode.internal.cache.entries.DiskEntry$Helper.basicUpdate(DiskEntry.java:955)
        at org.apache.geode.internal.cache.entries.DiskEntry$Helper.update(DiskEntry.java:867)
      • locked <0x00000000faaeb3d0> (a org.apache.geode.internal.cache.DiskId$PersistenceWithIntOffset)
        at org.apache.geode.internal.cache.entries.AbstractDiskRegionEntry.setValue(AbstractDiskRegionEntry.java:40)
        at org.apache.geode.internal.cache.entries.AbstractRegionEntry.setValueWithTombstoneCheck(AbstractRegionEntry.java:290)
        at org.apache.geode.internal.cache.EntryEventImpl.setNewValueInRegion(EntryEventImpl.java:1767)
        at org.apache.geode.internal.cache.EntryEventImpl.putExistingEntry(EntryEventImpl.java:1640)
        at org.apache.geode.internal.cache.map.RegionMapPut.updateEntry(RegionMapPut.java:485)
        at org.apache.geode.internal.cache.map.RegionMapPut.createOrUpdateEntry(RegionMapPut.java:256)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.doPutAndDeliverEvent(AbstractRegionMapPut.java:300)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut$$Lambda$420/0x0000000100a2b440.run(Unknown Source)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.runWithIndexUpdatingInProgress(AbstractRegionMapPut.java:308)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.doPutIfPreconditionsSatisified(AbstractRegionMapPut.java:296)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.doPutOnSynchronizedRegionEntry(AbstractRegionMapPut.java:282)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.doPutOnRegionEntryInMap(AbstractRegionMapPut.java:273)
      • locked <0x00000000faaeb388> (a org.apache.geode.internal.cache.entries.VersionedThinDiskRegionEntryHeapStringKey2)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.addRegionEntryToMapAndDoPut(AbstractRegionMapPut.java:251)
      • locked <0x00000000faaeb388> (a org.apache.geode.internal.cache.entries.VersionedThinDiskRegionEntryHeapStringKey2)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.doPutRetryingIfNeeded(AbstractRegionMapPut.java:216)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut$$Lambda$419/0x0000000100a2b040.run(Unknown Source)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.doWithIndexInUpdateMode(AbstractRegionMapPut.java:198)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.doPut(AbstractRegionMapPut.java:180)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut$$Lambda$418/0x0000000100a2ac40.run(Unknown Source)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.runWhileLockedForCacheModification(AbstractRegionMapPut.java:119)
        at org.apache.geode.internal.cache.map.RegionMapPut.runWhileLockedForCacheModification(RegionMapPut.java:161)
        at org.apache.geode.internal.cache.map.AbstractRegionMapPut.put(AbstractRegionMapPut.java:169)
        at org.apache.geode.internal.cache.AbstractRegionMap.basicPut(AbstractRegionMap.java:2036)
        at org.apache.geode.internal.cache.LocalRegion.virtualPut(LocalRegion.java:5600)
        at org.apache.geode.internal.cache.DistributedRegion.virtualPut(DistributedRegion.java:393)
        at org.apache.geode.internal.cache.LocalRegion.virtualPut(LocalRegion.java:5578)
        at org.apache.geode.internal.cache.LocalRegionDataView.putEntry(LocalRegionDataView.java:157)
        at org.apache.geode.internal.cache.LocalRegion.basicPut(LocalRegion.java:5036)
        at org.apache.geode.internal.cache.LocalRegion.validatedPut(LocalRegion.java:1635)
        at org.apache.geode.internal.cache.LocalRegion.put(LocalRegion.java:1622)
        at diskRecovery.RecoveryTest.updateEntry(RecoveryTest.java:3341)
        at diskRecovery.RecoveryTest.updateEntry(RecoveryTest.java:3318)
        at diskRecovery.RecoveryTest.doOperations(RecoveryTest.java:2929)
        at diskRecovery.RecoveryTest.concRecoverLatestResponder(RecoveryTest.java:2640)
        at diskRecovery.RecoveryTest.HydraTask_concRecoverLatestResponder(RecoveryTest.java:500)
        at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@11.0.9/Native Method)
        at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@11.0.9/NativeMethodAccessorImpl.java:62)
        at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@11.0.9/DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(java.base@11.0.9/Method.java:566)
        at hydra.MethExecutor.execute(MethExecutor.java:173)
        at hydra.MethExecutor.execute(MethExecutor.java:141)
        at hydra.TestTask.execute(TestTask.java:197)
        at hydra.RemoteTestModule$1.run(RemoteTestModule.java:213)

      Attachments

        Activity

          githubbot ASF GitHub Bot added a comment -

          pivotal-eshu opened a new pull request #6051:
          URL: https://github.com/apache/geode/pull/6051

          • Do not process DistributedCacheOperation in-line if scope is DISTRIBUTED_NO_ACK.
          • This is to solve a potential deadlock. The p2p reader thread could be blocked
            on synchronized lock of an entry, and could not handle the DLock GRANT message
            which is needed by another thread holding the synchronized lock.

          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:

              1. 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?
              1. 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

          githubbot ASF GitHub Bot added a comment - pivotal-eshu opened a new pull request #6051: URL: https://github.com/apache/geode/pull/6051 Do not process DistributedCacheOperation in-line if scope is DISTRIBUTED_NO_ACK. This is to solve a potential deadlock. The p2p reader thread could be blocked on synchronized lock of an entry, and could not handle the DLock GRANT message which is needed by another thread holding the synchronized lock. 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
          eshu Eric Shu added a comment -

          The issue is caused by the p2p reader thread (P2P message reader for rs-GEM-3166-PL1535a2i32xlarge-hydra-client-36(persistgemfire8_host1_8586:8586) was blocked on a synchronized lock and not able to handle the DLock GRANT message. The DLock is needed by the thread holding the synchronized lock.
          This issue occurs with scope of DISTRIBUTED_NO_ACK only. As the thread sending the DistributedCacheOperation does not wait for the reply from remote node, and it sends the GRANT message immediately afterwards.

          warn 2021/02/01 16:16:32.301 PST persistgemfire9_host1_8517 <ThreadsMonitor> tid=0x1d] Thread 81 (0x51) is stuck
          
          [warn 2021/02/01 16:16:32.307 PST persistgemfire9_host1_8517 <ThreadsMonitor> tid=0x1d] Thread <81> (0x51) that was executed at <01 Feb 2021 16:15:32 PST> has been stuck for <60.0 seconds> and number of thread monitor iteration <1>
          Thread Name <P2P message reader for rs-GEM-3166-PL1535a2i32xlarge-hydra-client-36(persistgemfire8_host1_8586:8586)<ec><v51>:41006 unshared ordered uid=1036 dom #1 local port=47207 remote port=42068> state <BLOCKED>
          Waiting on <org.apache.geode.internal.cache.entries.VersionedThinDiskRegionEntryHeapStringKey2@1699c3cf>
          Owned By <vm_10_thr_29_persist9_host1_8517> with ID <1530>
          Executor Group <P2PReaderExecutor>
          Monitored metric <ResourceManagerStats.numThreadsStuck>
          Thread stack:
          org.apache.geode.internal.cache.map.RegionMapDestroy.handleExistingRegionEntry(RegionMapDestroy.java:214)
          org.apache.geode.internal.cache.map.RegionMapDestroy.destroy(RegionMapDestroy.java:152)
          org.apache.geode.internal.cache.AbstractRegionMap.destroy(AbstractRegionMap.java:969)
          org.apache.geode.internal.cache.LocalRegion.mapDestroy(LocalRegion.java:6505)
          org.apache.geode.internal.cache.LocalRegion.mapDestroy(LocalRegion.java:6479)
          org.apache.geode.internal.cache.LocalRegionDataView.destroyExistingEntry(LocalRegionDataView.java:59)
          org.apache.geode.internal.cache.LocalRegion.basicDestroy(LocalRegion.java:6430)
          org.apache.geode.internal.cache.DistributedRegion.basicDestroy(DistributedRegion.java:1730)
          org.apache.geode.internal.cache.DestroyOperation$DestroyMessage.operateOnRegion(DestroyOperation.java:88)
          org.apache.geode.internal.cache.DistributedCacheOperation$CacheOperationMessage.basicProcess(DistributedCacheOperation.java:1208)
          org.apache.geode.internal.cache.DistributedCacheOperation$CacheOperationMessage.process(DistributedCacheOperation.java:1110)
          org.apache.geode.distributed.internal.DistributionMessage.scheduleAction(DistributionMessage.java:376)
          org.apache.geode.distributed.internal.DistributionMessage.schedule(DistributionMessage.java:432)
          org.apache.geode.distributed.internal.ClusterDistributionManager.scheduleIncomingMessage(ClusterDistributionManager.java:2070)
          org.apache.geode.distributed.internal.ClusterDistributionManager.handleIncomingDMsg(ClusterDistributionManager.java:1832)
          org.apache.geode.distributed.internal.ClusterDistributionManager$$Lambda$102/1096792171.messageReceived(Unknown Source)
          org.apache.geode.distributed.internal.membership.gms.GMSMembership.dispatchMessage(GMSMembership.java:925)
          org.apache.geode.distributed.internal.membership.gms.GMSMembership.handleOrDeferMessage(GMSMembership.java:856)
          org.apache.geode.distributed.internal.membership.gms.GMSMembership.processMessage(GMSMembership.java:1198)
          org.apache.geode.distributed.internal.DistributionImpl$MyDCReceiver.messageReceived(DistributionImpl.java:828)
          org.apache.geode.distributed.internal.direct.DirectChannel.receive(DirectChannel.java:614)
          org.apache.geode.internal.tcp.TCPConduit.messageReceived(TCPConduit.java:679)
          org.apache.geode.internal.tcp.Connection.dispatchMessage(Connection.java:3268)
          org.apache.geode.internal.tcp.Connection.readMessage(Connection.java:2993)
          org.apache.geode.internal.tcp.Connection.processInputBuffer(Connection.java:2797)
          org.apache.geode.internal.tcp.Connection.readMessages(Connection.java:1651)
          org.apache.geode.internal.tcp.Connection.run(Connection.java:1482)
          java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
          java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
          java.lang.Thread.run(Thread.java:748)
          Lock owner thread stack
          sun.misc.Unsafe.park(Native Method)
          
          Lock owner thread stack
          sun.misc.Unsafe.park(Native Method)
          java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:215)
          java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedNanos(AbstractQueuedSynchronizer.java:1037)
          java.util.concurrent.locks.AbstractQueuedSynchronizer.tryAcquireSharedNanos(AbstractQueuedSynchronizer.java:1328)
          java.util.concurrent.CountDownLatch.await(CountDownLatch.java:277)
          org.apache.geode.internal.util.concurrent.StoppableCountDownLatch.await(StoppableCountDownLatch.java:72)
          org.apache.geode.distributed.internal.ReplyProcessor21.basicWait(ReplyProcessor21.java:723)
          org.apache.geode.distributed.internal.ReplyProcessor21.waitForRepliesUninterruptibly(ReplyProcessor21.java:794)
          org.apache.geode.distributed.internal.ReplyProcessor21.waitForRepliesUninterruptibly(ReplyProcessor21.java:771)
          org.apache.geode.distributed.internal.ReplyProcessor21.waitForRepliesUninterruptibly(ReplyProcessor21.java:857)
          org.apache.geode.distributed.internal.locks.DLockRequestProcessor.requestLock(DLockRequestProcessor.java:238)
          org.apache.geode.distributed.internal.locks.DLockService.lockInterruptibly(DLockService.java:1505)
          org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1271)
          org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1262)
          org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1257)
          org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1253)
          org.apache.geode.pdx.internal.PeerTypeRegistration.lock(PeerTypeRegistration.java:314)
          org.apache.geode.pdx.internal.PeerTypeRegistration.defineEnum(PeerTypeRegistration.java:646)
          org.apache.geode.pdx.internal.PeerTypeRegistration.getEnumId(PeerTypeRegistration.java:601)
          org.apache.geode.pdx.internal.TypeRegistry.getEnumId(TypeRegistry.java:363)
          org.apache.geode.internal.InternalDataSerializer.writePdxEnum(InternalDataSerializer.java:2071)
          org.apache.geode.internal.InternalDataSerializer.writeUserObject(InternalDataSerializer.java:1610)
          org.apache.geode.internal.InternalDataSerializer.writeWellKnownObject(InternalDataSerializer.java:1517)
          org.apache.geode.internal.InternalDataSerializer.basicWriteObject(InternalDataSerializer.java:2034)
          org.apache.geode.pdx.internal.PdxOutputStream.writeObject(PdxOutputStream.java:72)
          org.apache.geode.pdx.internal.PdxWriterImpl.writeObject(PdxWriterImpl.java:341)
          org.apache.geode.pdx.internal.PdxWriterImpl.writeObject(PdxWriterImpl.java:330)
          util.VersionedValueHolder.myToData(VersionedValueHolder.java:227)
          util.PdxVersionedValueHolder.toData(PdxVersionedValueHolder.java:84)
          org.apache.geode.internal.InternalDataSerializer.writePdx(InternalDataSerializer.java:2794)
          org.apache.geode.internal.InternalDataSerializer.basicWriteObject(InternalDataSerializer.java:2011)
          org.apache.geode.DataSerializer.writeObject(DataSerializer.java:2839)
          org.apache.geode.internal.util.BlobHelper.serializeToBlob(BlobHelper.java:54)
          org.apache.geode.internal.cache.EntryEventImpl.serialize(EntryEventImpl.java:2092)
          org.apache.geode.internal.cache.EntryEventImpl.serialize(EntryEventImpl.java:2078)
          org.apache.geode.internal.cache.entries.DiskEntry$Helper.createValueWrapper(DiskEntry.java:768)
          org.apache.geode.internal.cache.entries.DiskEntry$Helper.basicUpdate(DiskEntry.java:955)
          org.apache.geode.internal.cache.entries.DiskEntry$Helper.update(DiskEntry.java:867)
          org.apache.geode.internal.cache.entries.AbstractDiskRegionEntry.setValue(AbstractDiskRegionEntry.java:40)
          org.apache.geode.internal.cache.entries.AbstractRegionEntry.setValueWithTombstoneCheck(AbstractRegionEntry.java:290)
          
          eshu Eric Shu added a comment - The issue is caused by the p2p reader thread (P2P message reader for rs-GEM-3166-PL1535a2i32xlarge-hydra-client-36(persistgemfire8_host1_8586:8586) was blocked on a synchronized lock and not able to handle the DLock GRANT message. The DLock is needed by the thread holding the synchronized lock. This issue occurs with scope of DISTRIBUTED_NO_ACK only. As the thread sending the DistributedCacheOperation does not wait for the reply from remote node, and it sends the GRANT message immediately afterwards. warn 2021/02/01 16:16:32.301 PST persistgemfire9_host1_8517 <ThreadsMonitor> tid=0x1d] Thread 81 (0x51) is stuck [warn 2021/02/01 16:16:32.307 PST persistgemfire9_host1_8517 <ThreadsMonitor> tid=0x1d] Thread <81> (0x51) that was executed at <01 Feb 2021 16:15:32 PST> has been stuck for <60.0 seconds> and number of thread monitor iteration <1> Thread Name <P2P message reader for rs-GEM-3166-PL1535a2i32xlarge-hydra-client-36(persistgemfire8_host1_8586:8586)<ec><v51>:41006 unshared ordered uid=1036 dom #1 local port=47207 remote port=42068> state <BLOCKED> Waiting on <org.apache.geode.internal.cache.entries.VersionedThinDiskRegionEntryHeapStringKey2@1699c3cf> Owned By <vm_10_thr_29_persist9_host1_8517> with ID <1530> Executor Group <P2PReaderExecutor> Monitored metric <ResourceManagerStats.numThreadsStuck> Thread stack: org.apache.geode.internal.cache.map.RegionMapDestroy.handleExistingRegionEntry(RegionMapDestroy.java:214) org.apache.geode.internal.cache.map.RegionMapDestroy.destroy(RegionMapDestroy.java:152) org.apache.geode.internal.cache.AbstractRegionMap.destroy(AbstractRegionMap.java:969) org.apache.geode.internal.cache.LocalRegion.mapDestroy(LocalRegion.java:6505) org.apache.geode.internal.cache.LocalRegion.mapDestroy(LocalRegion.java:6479) org.apache.geode.internal.cache.LocalRegionDataView.destroyExistingEntry(LocalRegionDataView.java:59) org.apache.geode.internal.cache.LocalRegion.basicDestroy(LocalRegion.java:6430) org.apache.geode.internal.cache.DistributedRegion.basicDestroy(DistributedRegion.java:1730) org.apache.geode.internal.cache.DestroyOperation$DestroyMessage.operateOnRegion(DestroyOperation.java:88) org.apache.geode.internal.cache.DistributedCacheOperation$CacheOperationMessage.basicProcess(DistributedCacheOperation.java:1208) org.apache.geode.internal.cache.DistributedCacheOperation$CacheOperationMessage.process(DistributedCacheOperation.java:1110) org.apache.geode.distributed.internal.DistributionMessage.scheduleAction(DistributionMessage.java:376) org.apache.geode.distributed.internal.DistributionMessage.schedule(DistributionMessage.java:432) org.apache.geode.distributed.internal.ClusterDistributionManager.scheduleIncomingMessage(ClusterDistributionManager.java:2070) org.apache.geode.distributed.internal.ClusterDistributionManager.handleIncomingDMsg(ClusterDistributionManager.java:1832) org.apache.geode.distributed.internal.ClusterDistributionManager$$Lambda$102/1096792171.messageReceived(Unknown Source) org.apache.geode.distributed.internal.membership.gms.GMSMembership.dispatchMessage(GMSMembership.java:925) org.apache.geode.distributed.internal.membership.gms.GMSMembership.handleOrDeferMessage(GMSMembership.java:856) org.apache.geode.distributed.internal.membership.gms.GMSMembership.processMessage(GMSMembership.java:1198) org.apache.geode.distributed.internal.DistributionImpl$MyDCReceiver.messageReceived(DistributionImpl.java:828) org.apache.geode.distributed.internal.direct.DirectChannel.receive(DirectChannel.java:614) org.apache.geode.internal.tcp.TCPConduit.messageReceived(TCPConduit.java:679) org.apache.geode.internal.tcp.Connection.dispatchMessage(Connection.java:3268) org.apache.geode.internal.tcp.Connection.readMessage(Connection.java:2993) org.apache.geode.internal.tcp.Connection.processInputBuffer(Connection.java:2797) org.apache.geode.internal.tcp.Connection.readMessages(Connection.java:1651) org.apache.geode.internal.tcp.Connection.run(Connection.java:1482) java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) java.lang.Thread.run(Thread.java:748) Lock owner thread stack sun.misc.Unsafe.park(Native Method) Lock owner thread stack sun.misc.Unsafe.park(Native Method) java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:215) java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedNanos(AbstractQueuedSynchronizer.java:1037) java.util.concurrent.locks.AbstractQueuedSynchronizer.tryAcquireSharedNanos(AbstractQueuedSynchronizer.java:1328) java.util.concurrent.CountDownLatch.await(CountDownLatch.java:277) org.apache.geode.internal.util.concurrent.StoppableCountDownLatch.await(StoppableCountDownLatch.java:72) org.apache.geode.distributed.internal.ReplyProcessor21.basicWait(ReplyProcessor21.java:723) org.apache.geode.distributed.internal.ReplyProcessor21.waitForRepliesUninterruptibly(ReplyProcessor21.java:794) org.apache.geode.distributed.internal.ReplyProcessor21.waitForRepliesUninterruptibly(ReplyProcessor21.java:771) org.apache.geode.distributed.internal.ReplyProcessor21.waitForRepliesUninterruptibly(ReplyProcessor21.java:857) org.apache.geode.distributed.internal.locks.DLockRequestProcessor.requestLock(DLockRequestProcessor.java:238) org.apache.geode.distributed.internal.locks.DLockService.lockInterruptibly(DLockService.java:1505) org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1271) org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1262) org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1257) org.apache.geode.distributed.internal.locks.DLockService.lock(DLockService.java:1253) org.apache.geode.pdx.internal.PeerTypeRegistration.lock(PeerTypeRegistration.java:314) org.apache.geode.pdx.internal.PeerTypeRegistration.defineEnum(PeerTypeRegistration.java:646) org.apache.geode.pdx.internal.PeerTypeRegistration.getEnumId(PeerTypeRegistration.java:601) org.apache.geode.pdx.internal.TypeRegistry.getEnumId(TypeRegistry.java:363) org.apache.geode.internal.InternalDataSerializer.writePdxEnum(InternalDataSerializer.java:2071) org.apache.geode.internal.InternalDataSerializer.writeUserObject(InternalDataSerializer.java:1610) org.apache.geode.internal.InternalDataSerializer.writeWellKnownObject(InternalDataSerializer.java:1517) org.apache.geode.internal.InternalDataSerializer.basicWriteObject(InternalDataSerializer.java:2034) org.apache.geode.pdx.internal.PdxOutputStream.writeObject(PdxOutputStream.java:72) org.apache.geode.pdx.internal.PdxWriterImpl.writeObject(PdxWriterImpl.java:341) org.apache.geode.pdx.internal.PdxWriterImpl.writeObject(PdxWriterImpl.java:330) util.VersionedValueHolder.myToData(VersionedValueHolder.java:227) util.PdxVersionedValueHolder.toData(PdxVersionedValueHolder.java:84) org.apache.geode.internal.InternalDataSerializer.writePdx(InternalDataSerializer.java:2794) org.apache.geode.internal.InternalDataSerializer.basicWriteObject(InternalDataSerializer.java:2011) org.apache.geode.DataSerializer.writeObject(DataSerializer.java:2839) org.apache.geode.internal.util.BlobHelper.serializeToBlob(BlobHelper.java:54) org.apache.geode.internal.cache.EntryEventImpl.serialize(EntryEventImpl.java:2092) org.apache.geode.internal.cache.EntryEventImpl.serialize(EntryEventImpl.java:2078) org.apache.geode.internal.cache.entries.DiskEntry$Helper.createValueWrapper(DiskEntry.java:768) org.apache.geode.internal.cache.entries.DiskEntry$Helper.basicUpdate(DiskEntry.java:955) org.apache.geode.internal.cache.entries.DiskEntry$Helper.update(DiskEntry.java:867) org.apache.geode.internal.cache.entries.AbstractDiskRegionEntry.setValue(AbstractDiskRegionEntry.java:40) org.apache.geode.internal.cache.entries.AbstractRegionEntry.setValueWithTombstoneCheck(AbstractRegionEntry.java:290)
          githubbot ASF GitHub Bot added a comment -

          lgtm-com[bot] commented on pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#issuecomment-784580721

          This pull request *fixes 2 alerts* when merging 4a854a6167ae6aa497348b2ad971deb72706b114 into 3a21c2852746f19755ac302f584ca5b8908eae2e - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-1683c4857c7e11b7f513a822db5bec3989d91494)

          *fixed alerts:*

          • 2 for Dereferenced variable may be null

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - lgtm-com [bot] commented on pull request #6051: URL: https://github.com/apache/geode/pull/6051#issuecomment-784580721 This pull request * fixes 2 alerts * when merging 4a854a6167ae6aa497348b2ad971deb72706b114 into 3a21c2852746f19755ac302f584ca5b8908eae2e - [view on LGTM.com] ( https://lgtm.com/projects/g/apache/geode/rev/pr-1683c4857c7e11b7f513a822db5bec3989d91494 ) * fixed alerts: * 2 for Dereferenced variable may be null ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          kirklund commented on a change in pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#discussion_r581467445

          ##########
          File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java
          ##########
          @@ -17,22 +17,68 @@
          import static org.assertj.core.api.Assertions.assertThat;
          import static org.junit.Assert.assertEquals;
          import static org.junit.Assert.assertTrue;
          +import static org.mockito.ArgumentMatchers.eq;
          +import static org.mockito.Mockito.doReturn;
          +import static org.mockito.Mockito.doThrow;
          import static org.mockito.Mockito.mock;
          +import static org.mockito.Mockito.never;
          +import static org.mockito.Mockito.spy;
          import static org.mockito.Mockito.times;
          import static org.mockito.Mockito.verify;
          import static org.mockito.Mockito.when;

          import java.util.HashMap;
          import java.util.Map;

          +import org.junit.Before;
          import org.junit.Test;
          +import org.mockito.ArgumentCaptor;

          +import org.apache.geode.cache.CacheClosedException;
          import org.apache.geode.cache.CacheEvent;
          +import org.apache.geode.cache.EntryNotFoundException;
          +import org.apache.geode.cache.Scope;
          +import org.apache.geode.distributed.internal.ClusterDistributionManager;
          +import org.apache.geode.distributed.internal.InternalDistributedSystem;
          +import org.apache.geode.distributed.internal.OperationExecutors;
          +import org.apache.geode.distributed.internal.ReplyException;
          import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
          import org.apache.geode.internal.cache.DistributedCacheOperation.CacheOperationMessage;
          import org.apache.geode.internal.cache.persistence.PersistentMemberID;
          +import org.apache.geode.internal.cache.versions.VersionTag;
          +import org.apache.geode.test.fake.Fakes;

          public class DistributedCacheOperationTest {
          + private TestCacheOperationMessage message;
          + private InternalDistributedMember sender;
          + private ClusterDistributionManager dm;
          + private LocalRegion region;
          + private VersionTag<?> versionTag;
          + private Scope scope;
          + private OperationExecutors executors;
          + private final int processorId = 1;
          +
          + @Before
          + public void setup() {
          + message = spy(new TestCacheOperationMessage());
          + sender = mock(InternalDistributedMember.class);
          + versionTag = mock(VersionTag.class);
          +
          + GemFireCacheImpl cache = Fakes.cache();

          Review comment:
          I'd really like to see us stop using `Fakes.cache()` in new unit tests so we can get rid of it. Can you please see if you can remove this and mock everything `DistributedCacheOperation` needs directly? Also, please try to use `InternalCache` and `InternalRegion` instead of `GemFireCacheImpl` and `LocalRegion`. Let me know if you'd like some help or want me to pair with you.

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - kirklund commented on a change in pull request #6051: URL: https://github.com/apache/geode/pull/6051#discussion_r581467445 ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java ########## @@ -17,22 +17,68 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.util.HashMap; import java.util.Map; +import org.junit.Before; import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.apache.geode.cache.CacheClosedException; import org.apache.geode.cache.CacheEvent; +import org.apache.geode.cache.EntryNotFoundException; +import org.apache.geode.cache.Scope; +import org.apache.geode.distributed.internal.ClusterDistributionManager; +import org.apache.geode.distributed.internal.InternalDistributedSystem; +import org.apache.geode.distributed.internal.OperationExecutors; +import org.apache.geode.distributed.internal.ReplyException; import org.apache.geode.distributed.internal.membership.InternalDistributedMember; import org.apache.geode.internal.cache.DistributedCacheOperation.CacheOperationMessage; import org.apache.geode.internal.cache.persistence.PersistentMemberID; +import org.apache.geode.internal.cache.versions.VersionTag; +import org.apache.geode.test.fake.Fakes; public class DistributedCacheOperationTest { + private TestCacheOperationMessage message; + private InternalDistributedMember sender; + private ClusterDistributionManager dm; + private LocalRegion region; + private VersionTag<?> versionTag; + private Scope scope; + private OperationExecutors executors; + private final int processorId = 1; + + @Before + public void setup() { + message = spy(new TestCacheOperationMessage()); + sender = mock(InternalDistributedMember.class); + versionTag = mock(VersionTag.class); + + GemFireCacheImpl cache = Fakes.cache(); Review comment: I'd really like to see us stop using `Fakes.cache()` in new unit tests so we can get rid of it. Can you please see if you can remove this and mock everything `DistributedCacheOperation` needs directly? Also, please try to use `InternalCache` and `InternalRegion` instead of `GemFireCacheImpl` and `LocalRegion`. Let me know if you'd like some help or want me to pair with you. ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          kirklund commented on pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#issuecomment-784606768

          The changes look great. The only change I'd really like to see is to avoid the use of `Fakes.cache()`.

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - kirklund commented on pull request #6051: URL: https://github.com/apache/geode/pull/6051#issuecomment-784606768 The changes look great. The only change I'd really like to see is to avoid the use of `Fakes.cache()`. ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          kirklund edited a comment on pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#issuecomment-784606768

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - kirklund edited a comment on pull request #6051: URL: https://github.com/apache/geode/pull/6051#issuecomment-784606768 ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          pivotal-eshu commented on a change in pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#discussion_r581494208

          ##########
          File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java
          ##########
          @@ -17,22 +17,68 @@
          import static org.assertj.core.api.Assertions.assertThat;
          import static org.junit.Assert.assertEquals;
          import static org.junit.Assert.assertTrue;
          +import static org.mockito.ArgumentMatchers.eq;
          +import static org.mockito.Mockito.doReturn;
          +import static org.mockito.Mockito.doThrow;
          import static org.mockito.Mockito.mock;
          +import static org.mockito.Mockito.never;
          +import static org.mockito.Mockito.spy;
          import static org.mockito.Mockito.times;
          import static org.mockito.Mockito.verify;
          import static org.mockito.Mockito.when;

          import java.util.HashMap;
          import java.util.Map;

          +import org.junit.Before;
          import org.junit.Test;
          +import org.mockito.ArgumentCaptor;

          +import org.apache.geode.cache.CacheClosedException;
          import org.apache.geode.cache.CacheEvent;
          +import org.apache.geode.cache.EntryNotFoundException;
          +import org.apache.geode.cache.Scope;
          +import org.apache.geode.distributed.internal.ClusterDistributionManager;
          +import org.apache.geode.distributed.internal.InternalDistributedSystem;
          +import org.apache.geode.distributed.internal.OperationExecutors;
          +import org.apache.geode.distributed.internal.ReplyException;
          import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
          import org.apache.geode.internal.cache.DistributedCacheOperation.CacheOperationMessage;
          import org.apache.geode.internal.cache.persistence.PersistentMemberID;
          +import org.apache.geode.internal.cache.versions.VersionTag;
          +import org.apache.geode.test.fake.Fakes;

          public class DistributedCacheOperationTest {
          + private TestCacheOperationMessage message;
          + private InternalDistributedMember sender;
          + private ClusterDistributionManager dm;
          + private LocalRegion region;
          + private VersionTag<?> versionTag;
          + private Scope scope;
          + private OperationExecutors executors;
          + private final int processorId = 1;
          +
          + @Before
          + public void setup() {
          + message = spy(new TestCacheOperationMessage());
          + sender = mock(InternalDistributedMember.class);
          + versionTag = mock(VersionTag.class);
          +
          + GemFireCacheImpl cache = Fakes.cache();

          Review comment:
          Removed the Fakes.
          As the product code method signature is using LocalRegion, using InternalRegion may need some casting. Will leave it as is, unless we need to change the product code.

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - pivotal-eshu commented on a change in pull request #6051: URL: https://github.com/apache/geode/pull/6051#discussion_r581494208 ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java ########## @@ -17,22 +17,68 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.util.HashMap; import java.util.Map; +import org.junit.Before; import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.apache.geode.cache.CacheClosedException; import org.apache.geode.cache.CacheEvent; +import org.apache.geode.cache.EntryNotFoundException; +import org.apache.geode.cache.Scope; +import org.apache.geode.distributed.internal.ClusterDistributionManager; +import org.apache.geode.distributed.internal.InternalDistributedSystem; +import org.apache.geode.distributed.internal.OperationExecutors; +import org.apache.geode.distributed.internal.ReplyException; import org.apache.geode.distributed.internal.membership.InternalDistributedMember; import org.apache.geode.internal.cache.DistributedCacheOperation.CacheOperationMessage; import org.apache.geode.internal.cache.persistence.PersistentMemberID; +import org.apache.geode.internal.cache.versions.VersionTag; +import org.apache.geode.test.fake.Fakes; public class DistributedCacheOperationTest { + private TestCacheOperationMessage message; + private InternalDistributedMember sender; + private ClusterDistributionManager dm; + private LocalRegion region; + private VersionTag<?> versionTag; + private Scope scope; + private OperationExecutors executors; + private final int processorId = 1; + + @Before + public void setup() { + message = spy(new TestCacheOperationMessage()); + sender = mock(InternalDistributedMember.class); + versionTag = mock(VersionTag.class); + + GemFireCacheImpl cache = Fakes.cache(); Review comment: Removed the Fakes. As the product code method signature is using LocalRegion, using InternalRegion may need some casting. Will leave it as is, unless we need to change the product code. ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          lgtm-com[bot] commented on pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#issuecomment-784649285

          This pull request *fixes 2 alerts* when merging 7addfe4c2c220d9963ec2060191547e329d800a2 into 3a21c2852746f19755ac302f584ca5b8908eae2e - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-f29a55f2fbac14a855834134cda854584c6d7ce7)

          *fixed alerts:*

          • 2 for Dereferenced variable may be null

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - lgtm-com [bot] commented on pull request #6051: URL: https://github.com/apache/geode/pull/6051#issuecomment-784649285 This pull request * fixes 2 alerts * when merging 7addfe4c2c220d9963ec2060191547e329d800a2 into 3a21c2852746f19755ac302f584ca5b8908eae2e - [view on LGTM.com] ( https://lgtm.com/projects/g/apache/geode/rev/pr-f29a55f2fbac14a855834134cda854584c6d7ce7 ) * fixed alerts: * 2 for Dereferenced variable may be null ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          DonalEvans commented on a change in pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#discussion_r582199184

          ##########
          File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java
          ##########
          @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError()

          { assertTrue(operation.endOperationInvoked); }

          + @Test
          + public void processReplacesVersionTagNullIDs() { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + }
          +
          + @Test
          + public void processSendsReplyIfAdminDM() { + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + }
          +
          + @Test
          + public void processSendsReplyIfLocalRegionIsNull() {
          + doReturn(null).when(message).getLocalRegionForProcessing(dm);
          +
          + message.process(dm);
          +
          + assertThat(message.closed).isTrue();
          + verify(message, never()).basicProcess(dm, region);
          + verify(message).sendReply(
          + eq(sender),

          Review comment:
          It's not necessary to wrap the arguments here with `eq()`, so this can be simplified.

          ##########
          File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java
          ##########
          @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); }

          + @Test
          + public void processReplacesVersionTagNullIDs()

          { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + }
          +
          + @Test
          + public void processSendsReplyIfAdminDM() {
          + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE);
          +
          + message.process(dm);
          +
          + verify(message, never()).basicProcess(dm, region);
          + verify(message).sendReply(
          + eq(sender),

          Review comment:
          It's not necessary to wrap the arguments here with `eq()`, so this can be simplified.

          ##########
          File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java
          ##########
          @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); }

          + @Test
          + public void processReplacesVersionTagNullIDs() {+ message.process(dm);++ verify(versionTag).replaceNullIDs(sender);+ }

          +
          + @Test
          + public void processSendsReplyIfAdminDM()

          { + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + }
          +
          + @Test
          + public void processSendsReplyIfLocalRegionIsNull() { + doReturn(null).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + }
          +
          + @Test
          + public void processSendsReplyIfGotCacheClosedException() {
          + CacheClosedException cacheClosedException = new CacheClosedException();
          + doThrow(cacheClosedException).when(message).getLocalRegionForProcessing(dm);
          +
          + message.process(dm);
          +
          + assertThat(message.closed).isTrue();

          Review comment:
          This would be simpler as `assertTrue()`.

          ##########
          File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java
          ##########
          @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); }

          + @Test
          + public void processReplacesVersionTagNullIDs() { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + }
          +
          + @Test
          + public void processSendsReplyIfAdminDM() {+ when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE);++ message.process(dm);++ verify(message, never()).basicProcess(dm, region);+ verify(message).sendReply(+ eq(sender),+ eq(processorId),+ eq(null),+ eq(dm));+ }

          +
          + @Test
          + public void processSendsReplyIfLocalRegionIsNull()

          { + doReturn(null).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + }
          +
          + @Test
          + public void processSendsReplyIfGotCacheClosedException() { + CacheClosedException cacheClosedException = new CacheClosedException(); + doThrow(cacheClosedException).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + }
          +
          + @Test
          + public void processSendsReplyExceptionIfGotRuntimeException() { + RuntimeException exception = new RuntimeException(); + doThrow(exception).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + ArgumentCaptor<ReplyException> captor = ArgumentCaptor.forClass(ReplyException.class); + verify(message).sendReply( + eq(sender), + eq(processorId), + captor.capture(), + eq(dm)); + assertThat(captor.getValue().getCause()).isSameAs(exception); + }
          +
          + @Test
          + public void processPerformsBasicProcessIfNotDistributedNoAck() { + when(scope.isDistributedNoAck()).thenReturn(false); + + message.process(dm); + + verify(message).basicProcess(dm, region); + verify(executors, never()).getWaitingThreadPool(); + }
          +
          + @Test
          + public void processUsesWaitingThreadPoolIfDistributedNoAck() { + when(scope.isDistributedNoAck()).thenReturn(true); + + message.process(dm); + + verify(executors).getWaitingThreadPool(); + }
          +
          + @Test
          + public void processDoesNotSendReplyIfDistributedNoAck() {
          + when(scope.isDistributedNoAck()).thenReturn(true);
          +
          + message.process(dm);
          +
          + verify(message, never()).sendReply(
          + eq(sender),

          Review comment:
          It's not necessary to use `eq()` for these arguments.

          ##########
          File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java
          ##########
          @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); }

          + @Test
          + public void processReplacesVersionTagNullIDs() { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + }
          +
          + @Test
          + public void processSendsReplyIfAdminDM() { + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + }
          +
          + @Test
          + public void processSendsReplyIfLocalRegionIsNull() {+ doReturn(null).when(message).getLocalRegionForProcessing(dm);++ message.process(dm);++ assertThat(message.closed).isTrue();+ verify(message, never()).basicProcess(dm, region);+ verify(message).sendReply(+ eq(sender),+ eq(processorId),+ eq(null),+ eq(dm));+ }

          +
          + @Test
          + public void processSendsReplyIfGotCacheClosedException() {
          + CacheClosedException cacheClosedException = new CacheClosedException();
          + doThrow(cacheClosedException).when(message).getLocalRegionForProcessing(dm);
          +
          + message.process(dm);
          +
          + assertThat(message.closed).isTrue();
          + verify(message, never()).basicProcess(dm, region);
          + verify(message).sendReply(
          + eq(sender),

          Review comment:
          It's not necessary to use `eq()` here.

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - DonalEvans commented on a change in pull request #6051: URL: https://github.com/apache/geode/pull/6051#discussion_r582199184 ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java ########## @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); } + @Test + public void processReplacesVersionTagNullIDs() { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + } + + @Test + public void processSendsReplyIfAdminDM() { + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + } + + @Test + public void processSendsReplyIfLocalRegionIsNull() { + doReturn(null).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), Review comment: It's not necessary to wrap the arguments here with `eq()`, so this can be simplified. ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java ########## @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); } + @Test + public void processReplacesVersionTagNullIDs() { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + } + + @Test + public void processSendsReplyIfAdminDM() { + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), Review comment: It's not necessary to wrap the arguments here with `eq()`, so this can be simplified. ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java ########## @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); } + @Test + public void processReplacesVersionTagNullIDs() {+ message.process(dm);++ verify(versionTag).replaceNullIDs(sender);+ } + + @Test + public void processSendsReplyIfAdminDM() { + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + } + + @Test + public void processSendsReplyIfLocalRegionIsNull() { + doReturn(null).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + } + + @Test + public void processSendsReplyIfGotCacheClosedException() { + CacheClosedException cacheClosedException = new CacheClosedException(); + doThrow(cacheClosedException).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); Review comment: This would be simpler as `assertTrue()`. ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java ########## @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); } + @Test + public void processReplacesVersionTagNullIDs() { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + } + + @Test + public void processSendsReplyIfAdminDM() {+ when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE);++ message.process(dm);++ verify(message, never()).basicProcess(dm, region);+ verify(message).sendReply(+ eq(sender),+ eq(processorId),+ eq(null),+ eq(dm));+ } + + @Test + public void processSendsReplyIfLocalRegionIsNull() { + doReturn(null).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + } + + @Test + public void processSendsReplyIfGotCacheClosedException() { + CacheClosedException cacheClosedException = new CacheClosedException(); + doThrow(cacheClosedException).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + } + + @Test + public void processSendsReplyExceptionIfGotRuntimeException() { + RuntimeException exception = new RuntimeException(); + doThrow(exception).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + ArgumentCaptor<ReplyException> captor = ArgumentCaptor.forClass(ReplyException.class); + verify(message).sendReply( + eq(sender), + eq(processorId), + captor.capture(), + eq(dm)); + assertThat(captor.getValue().getCause()).isSameAs(exception); + } + + @Test + public void processPerformsBasicProcessIfNotDistributedNoAck() { + when(scope.isDistributedNoAck()).thenReturn(false); + + message.process(dm); + + verify(message).basicProcess(dm, region); + verify(executors, never()).getWaitingThreadPool(); + } + + @Test + public void processUsesWaitingThreadPoolIfDistributedNoAck() { + when(scope.isDistributedNoAck()).thenReturn(true); + + message.process(dm); + + verify(executors).getWaitingThreadPool(); + } + + @Test + public void processDoesNotSendReplyIfDistributedNoAck() { + when(scope.isDistributedNoAck()).thenReturn(true); + + message.process(dm); + + verify(message, never()).sendReply( + eq(sender), Review comment: It's not necessary to use `eq()` for these arguments. ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java ########## @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); } + @Test + public void processReplacesVersionTagNullIDs() { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + } + + @Test + public void processSendsReplyIfAdminDM() { + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + } + + @Test + public void processSendsReplyIfLocalRegionIsNull() {+ doReturn(null).when(message).getLocalRegionForProcessing(dm);++ message.process(dm);++ assertThat(message.closed).isTrue();+ verify(message, never()).basicProcess(dm, region);+ verify(message).sendReply(+ eq(sender),+ eq(processorId),+ eq(null),+ eq(dm));+ } + + @Test + public void processSendsReplyIfGotCacheClosedException() { + CacheClosedException cacheClosedException = new CacheClosedException(); + doThrow(cacheClosedException).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), Review comment: It's not necessary to use `eq()` here. ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          pivotal-eshu commented on a change in pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#discussion_r582346253

          ##########
          File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java
          ##########
          @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError()

          { assertTrue(operation.endOperationInvoked); }

          + @Test
          + public void processReplacesVersionTagNullIDs()

          { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + }

          +
          + @Test
          + public void processSendsReplyIfAdminDM()

          { + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + }

          +
          + @Test
          + public void processSendsReplyIfLocalRegionIsNull()

          { + doReturn(null).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + }

          +
          + @Test
          + public void processSendsReplyIfGotCacheClosedException() {
          + CacheClosedException cacheClosedException = new CacheClosedException();
          + doThrow(cacheClosedException).when(message).getLocalRegionForProcessing(dm);
          +
          + message.process(dm);
          +
          + assertThat(message.closed).isTrue();

          Review comment:
          I believe there was a requirement that all new tests should use assertj methods.

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - pivotal-eshu commented on a change in pull request #6051: URL: https://github.com/apache/geode/pull/6051#discussion_r582346253 ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedCacheOperationTest.java ########## @@ -72,11 +114,112 @@ public void endOperationIsInvokedOnDistributionError() { assertTrue(operation.endOperationInvoked); } + @Test + public void processReplacesVersionTagNullIDs() { + message.process(dm); + + verify(versionTag).replaceNullIDs(sender); + } + + @Test + public void processSendsReplyIfAdminDM() { + when(dm.getDMType()).thenReturn(ClusterDistributionManager.ADMIN_ONLY_DM_TYPE); + + message.process(dm); + + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + } + + @Test + public void processSendsReplyIfLocalRegionIsNull() { + doReturn(null).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); + verify(message, never()).basicProcess(dm, region); + verify(message).sendReply( + eq(sender), + eq(processorId), + eq(null), + eq(dm)); + } + + @Test + public void processSendsReplyIfGotCacheClosedException() { + CacheClosedException cacheClosedException = new CacheClosedException(); + doThrow(cacheClosedException).when(message).getLocalRegionForProcessing(dm); + + message.process(dm); + + assertThat(message.closed).isTrue(); Review comment: I believe there was a requirement that all new tests should use assertj methods. ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          bschuchardt commented on a change in pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#discussion_r582366653

          ##########
          File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java
          ##########
          @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) {

          final LocalRegion lclRgn = getLocalRegionForProcessing(dm);
          sendReply = false;
          + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) {
          + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn));

          Review comment:
          this previously caught CancelException and set this.closed=true. That's no longer being done for distributed-no-ack regions. Why is that okay?

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - bschuchardt commented on a change in pull request #6051: URL: https://github.com/apache/geode/pull/6051#discussion_r582366653 ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java ########## @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) { final LocalRegion lclRgn = getLocalRegionForProcessing(dm); sendReply = false; + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) { + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn)); Review comment: this previously caught CancelException and set this.closed=true. That's no longer being done for distributed-no-ack regions. Why is that okay? ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          jchen21 commented on a change in pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#discussion_r582416387

          ##########
          File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java
          ##########
          @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) {

          final LocalRegion lclRgn = getLocalRegionForProcessing(dm);
          sendReply = false;
          + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) {
          + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn));

          Review comment:
          I have the same concern. When a thread runs, if it throws an exception that is not caught, then that exception will just silently kill the thread. This can make it very hard to diagnose what is happening. The current thread has a few `catch` blocks and a `finally` block (line 1115-1147). If the newly spawned thread does not have the `catch` and `finally` blocks, I would like to understand why.

          Another question is why use `getWaitingThreadPool()` and not the other pool, e.g. `getThreadPool()`?

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - jchen21 commented on a change in pull request #6051: URL: https://github.com/apache/geode/pull/6051#discussion_r582416387 ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java ########## @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) { final LocalRegion lclRgn = getLocalRegionForProcessing(dm); sendReply = false; + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) { + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn)); Review comment: I have the same concern. When a thread runs, if it throws an exception that is not caught, then that exception will just silently kill the thread. This can make it very hard to diagnose what is happening. The current thread has a few `catch` blocks and a `finally` block (line 1115-1147). If the newly spawned thread does not have the `catch` and `finally` blocks, I would like to understand why. Another question is why use `getWaitingThreadPool()` and not the other pool, e.g. `getThreadPool()`? ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          pivotal-eshu commented on a change in pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#discussion_r582427022

          ##########
          File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java
          ##########
          @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) {

          final LocalRegion lclRgn = getLocalRegionForProcessing(dm);
          sendReply = false;
          + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) {
          + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn));

          Review comment:
          In basicProcess call (using waiting thread pool for distributed-no-ack regions), CancelException is also caught and handled.
          catch (RegionDestroyedException ignore) {
          this.closed = true;
          if (logger.isDebugEnabled()) {
          logger.debug("{} Region destroyed: nothing to do", this);
          }
          } catch (CancelException ignore) {
          this.closed = true;
          if (logger.isDebugEnabled()) {
          logger.debug("{} Cancelled: nothing to do", this);
          }
          }

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - pivotal-eshu commented on a change in pull request #6051: URL: https://github.com/apache/geode/pull/6051#discussion_r582427022 ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java ########## @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) { final LocalRegion lclRgn = getLocalRegionForProcessing(dm); sendReply = false; + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) { + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn)); Review comment: In basicProcess call (using waiting thread pool for distributed-no-ack regions), CancelException is also caught and handled. catch (RegionDestroyedException ignore) { this.closed = true; if (logger.isDebugEnabled()) { logger.debug("{} Region destroyed: nothing to do", this); } } catch (CancelException ignore) { this.closed = true; if (logger.isDebugEnabled()) { logger.debug("{} Cancelled: nothing to do", this); } } ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          jchen21 commented on a change in pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#discussion_r583317242

          ##########
          File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java
          ##########
          @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) {

          final LocalRegion lclRgn = getLocalRegionForProcessing(dm);
          sendReply = false;
          + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) {
          + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn));

          Review comment:
          Good point. For `basicProcess()`, if debug is not enabled, some exceptions are caught, but not logged, although some flags are set, when the exceptions are caught. Will the new thread fail silently with such exceptions, if debug is not enabled?
          And even for `process()`, logging the exceptions also depends on whether debug is enabled. I am not sure whether this is by design. If debug is not enabled, without the log message, it is hard to analyze the failures.

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - jchen21 commented on a change in pull request #6051: URL: https://github.com/apache/geode/pull/6051#discussion_r583317242 ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java ########## @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) { final LocalRegion lclRgn = getLocalRegionForProcessing(dm); sendReply = false; + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) { + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn)); Review comment: Good point. For `basicProcess()`, if debug is not enabled, some exceptions are caught, but not logged, although some flags are set, when the exceptions are caught. Will the new thread fail silently with such exceptions, if debug is not enabled? And even for `process()`, logging the exceptions also depends on whether debug is enabled. I am not sure whether this is by design. If debug is not enabled, without the log message, it is hard to analyze the failures. ---------------------------------------------------------------- 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
          githubbot ASF GitHub Bot added a comment -

          pivotal-eshu commented on a change in pull request #6051:
          URL: https://github.com/apache/geode/pull/6051#discussion_r585803394

          ##########
          File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java
          ##########
          @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) {

          final LocalRegion lclRgn = getLocalRegionForProcessing(dm);
          sendReply = false;
          + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) {
          + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn));

          Review comment:
          I believe it is intentional. We do not want to pollute the user logging.

          ----------------------------------------------------------------
          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

          githubbot ASF GitHub Bot added a comment - pivotal-eshu commented on a change in pull request #6051: URL: https://github.com/apache/geode/pull/6051#discussion_r585803394 ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedCacheOperation.java ########## @@ -1107,6 +1107,10 @@ protected void process(final ClusterDistributionManager dm) { final LocalRegion lclRgn = getLocalRegionForProcessing(dm); sendReply = false; + if (lclRgn != null && lclRgn.getScope().isDistributedNoAck()) { + dm.getExecutors().getWaitingThreadPool().execute(() -> basicProcess(dm, lclRgn)); Review comment: I believe it is intentional. We do not want to pollute the user logging. ---------------------------------------------------------------- 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

          Commit 97700d3858d6dbc71c7d51769d5a3411d5bb7a5e in geode's branch refs/heads/develop from Eric Shu
          [ https://gitbox.apache.org/repos/asf?p=geode.git;h=97700d3 ]

          GEODE-8862: Send grant message to remote node using waiting thread pool. (#6361)

          • In D_NO_ACK region, remote P2P reader thread may still be blocked handling the
            previous distriubted operation, and not able to handle the GRANT message.
          • Use executor waiting thread pool can avoid the potenial dead lock
          jira-bot ASF subversion and git services added a comment - Commit 97700d3858d6dbc71c7d51769d5a3411d5bb7a5e in geode's branch refs/heads/develop from Eric Shu [ https://gitbox.apache.org/repos/asf?p=geode.git;h=97700d3 ] GEODE-8862 : Send grant message to remote node using waiting thread pool. (#6361) In D_NO_ACK region, remote P2P reader thread may still be blocked handling the previous distriubted operation, and not able to handle the GRANT message. Use executor waiting thread pool can avoid the potenial dead lock

          People

            eshu Eric Shu
            eshu Eric Shu
            Votes:
            0 Vote for this issue
            Watchers:
            Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack

                Issue deployment