[GitHub] [geode] kohlmu-pivotal opened a new pull request #5548: GEODE-8466: Introduction of ClassLoaderService as replacement
kohlmu-pivotal opened a new pull request #5548: URL: https://github.com/apache/geode/pull/5548 for the direct feature usage of ClassLoader and ClassLoaderPath 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 d...@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: us...@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
sabbeyPivotal commented on pull request #5544: URL: https://github.com/apache/geode/pull/5544#issuecomment-698047025 Distributed test failure is unrelated to this PR. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
jinmeiliao commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493961781
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
Review comment:
Not using += anymore.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #645: GEODE-8469: Enable no-missing-variable-declarations
pivotal-jbarrett commented on pull request #645: URL: https://github.com/apache/geode-native/pull/645#issuecomment-697999232 > I think we can diagnose the test problem with a couple of (very tedious) hours of debugging, which is a lot better than rewriting the test. I think we're much closer to tackling the job of rewriting _all_ of the old tests, tho. I would just not apply this check to the legacy tests. They really aren't worth cleaning up. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on pull request #5543: GEODE-8496: fix rest management test after dependency bump
onichols-pivotal commented on pull request #5543: URL: https://github.com/apache/geode/pull/5543#issuecomment-697997186 Fixed by #5547 thanks! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] onichols-pivotal closed pull request #5543: GEODE-8496: fix rest management test after dependency bump
onichols-pivotal closed pull request #5543: URL: https://github.com/apache/geode/pull/5543 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] onichols-pivotal merged pull request #5547: GEODE-8496: fix rest management test after dependency bump
onichols-pivotal merged pull request #5547: URL: https://github.com/apache/geode/pull/5547 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] onichols-pivotal merged pull request #5540: Bump mockito from 3.3.3 to 3.5.11
onichols-pivotal merged pull request #5540: URL: https://github.com/apache/geode/pull/5540 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
jinmeiliao commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493896136
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
+ } else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+collectionTime += value.longValue();
+ }
}
}
@Override
public Number getStatistic(String statName) {
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- return getCollections();
+ return collections;
}
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- return getCollectionTime();
+ return collectionTime;
}
-
return 0;
}
@Override
public void handleNotification(StatisticsNotification notification) {
-decreasePrevValues(statsMap);
-
+// sum up all the count and all the time in the stats included in this
notification
+long totalCount = 0;
+long totalTime = 0;
for (StatisticId statId : notification) {
StatisticDescriptor descriptor = statId.getStatisticDescriptor();
String name = descriptor.getName();
Number value;
-
try {
value = notification.getValue(statId);
} catch (StatisticNotFoundException e) {
value = 0;
}
-
log(name, value);
- increaseStats(name, value);
- statsMap.put(name, value);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+totalCount += value.longValue();
+ }
+
+ else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+totalTime += value.longValue();
+ }
}
+
+collections = totalCount;
+collectionTime = totalTime;
}
Review comment:
> > then what would those count/time mean though. It's not right to
report just a single one of them. It's not right to sum them up either then
>
> The stats notification stuff is specifically designed to report only
changes. As a result, old gen and new gen CGs are almost always reported
independently (unless they both happen to change during a single sample).
>
> I think that's why the old code was doing the weird stuff. It was trying
to track the previous value for each kind of GC, so that when a new value
arrived, it could add in just the change in that kind of GC. Decrementing and
incrementing was a puzzling way to do that, but that was the goal.
>
> Here's a way that seems simpler to me than decrementing/incrementing. I'll
use collections as an example, but it's the same for collectionTime.
>
> * Create a map that records the latest value for each GC type.
> * Create a field that records the sum of values for all types. This is
what gets reported via getCollections()
> * When a new kind of GC is added via addStatisticsToMonitor()…
>
> 1. Store the initial value for that GC type in the map.
> 2. Sum the values of all GC types in the map, and store the sum in the
field.
> * When a notification arrives…
>
> 1. Store the new value for that GC type in the map.
> 2. Sum the values of all GC types in the map, and store the sum in the
field.
[GitHub] [geode] jinmeiliao commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
jinmeiliao commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493896436
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
Review comment:
I think we are only interested in these two.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493890530
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
Review comment:
Those are currently the only two descriptors in the GC statistics type,
and the only two values reported by GarbageCollectorMXBean (the ultimate source
of the values). If another descriptor were to be added to the statistics type
in the future, we'd want to review it before deciding how `GCStatsMonitor`
should handle it. So I think it's appropriate to ignore any descriptors we
don't recognize.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey opened a new pull request #655: GEODE-8524: Add support for KEY_SET and GET_ALL_70 messages
pdxcodemonkey opened a new pull request #655: URL: https://github.com/apache/geode-native/pull/655 Found a log file this morning from an app that was calling `getKeys()` and `getAll()`, and discovered these message that hadn't yet been added to the parser. @alb3rtobr @mreddington @davebarnes97 @karensmolermiller @dihardman This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5516: GEODE-7679 Partitioned Region clear is successful while region is being altered
jchen21 commented on a change in pull request #5516:
URL: https://github.com/apache/geode/pull/5516#discussion_r493858900
##
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithAlterRegionDUnitTest.java
##
@@ -0,0 +1,424 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.stream.IntStream;
+
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.AttributesMutator;
+import org.apache.geode.cache.CacheListener;
+import org.apache.geode.cache.CacheLoader;
+import org.apache.geode.cache.CacheLoaderException;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.EntryEvent;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.LoaderHelper;
+import org.apache.geode.cache.PartitionedRegionPartialClearException;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.distributed.internal.DistributionMessageObserver;
+import org.apache.geode.test.dunit.AsyncInvocation;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class PartitionedRegionClearWithAlterRegionDUnitTest implements
Serializable {
+
+ @Rule
+ public DistributedRule distributedRule = new DistributedRule();
+
+ @Rule
+ public CacheRule cacheRule = new CacheRule();
+
+ @Rule
+ public ExecutorServiceRule executorServiceRule = new ExecutorServiceRule();
+
+ private VM server1;
+
+ private VM server2;
+
+ private VM server3;
+
+ private static final String REGION_NAME = "testRegion";
+
+ private static final int NUM_ENTRIES = 100;
+
+ private void initialize() {
+server1 = VM.getVM(0);
+server2 = VM.getVM(1);
+
+server1.invoke(() -> {
+ cacheRule.createCache();
+
cacheRule.getCache().createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(REGION_NAME);
+});
+
+server2.invoke(() -> {
+ cacheRule.createCache();
+
cacheRule.getCache().createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(REGION_NAME);
+});
+
+server1.invoke(() -> {
+ populateRegion();
+ Region region = cacheRule.getCache().getRegion(REGION_NAME);
+ assertThat(region.size()).isEqualTo(NUM_ENTRIES);
+});
+
+server2.invoke(() -> {
+ Region region = cacheRule.getCache().getRegion(REGION_NAME);
+ assertThat(region.size()).isEqualTo(NUM_ENTRIES);
+});
+ }
+
+ @Test
+ public void testClearRegionWhileAddingCacheLoader() throws
InterruptedException {
+initialize();
+
+AsyncInvocation asyncInvocation1 = server1.invokeAsync(() -> {
+ cacheRule.getCache().getRegion(REGION_NAME).clear();
+
assertThat(cacheRule.getCache().getRegion(REGION_NAME).size()).isEqualTo(0);
+});
+
+AsyncInvocation asyncInvocation2 = server2.invokeAsync(() -> {
+ alterRegionSetCacheLoader();
+});
+
+asyncInvocation1.await();
+asyncInvocation2.await();
+ }
+
+
+
+ @Test
+ public void testClearRegionWhileAddingCacheWriter() throws
InterruptedException {
+initialize();
+
+AsyncInvocation asyncInvocation1 = server1.invokeAsync(() -> {
+ cacheRule.getCache().getRegion(REGION_NAME).clear();
+
assertThat(cacheRule.getCache().getRegion(REGION_NAME).size()).isEqualTo(0);
+});
+
+AsyncInvocation asyncInvocation2 = server2.invokeAsync(() -> {
+ alterRegionSetCacheWriter();
+});
+
+asyncInvocation1.await();
+asyncInvocation2.await();
+ }
Review comment:
It is verified at line 123 and line 330.
[GitHub] [geode] jchen21 commented on a change in pull request #5516: GEODE-7679 Partitioned Region clear is successful while region is being altered
jchen21 commented on a change in pull request #5516:
URL: https://github.com/apache/geode/pull/5516#discussion_r493858361
##
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithAlterRegionDUnitTest.java
##
@@ -0,0 +1,424 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.stream.IntStream;
+
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.AttributesMutator;
+import org.apache.geode.cache.CacheListener;
+import org.apache.geode.cache.CacheLoader;
+import org.apache.geode.cache.CacheLoaderException;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.EntryEvent;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.LoaderHelper;
+import org.apache.geode.cache.PartitionedRegionPartialClearException;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.distributed.internal.DistributionMessageObserver;
+import org.apache.geode.test.dunit.AsyncInvocation;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class PartitionedRegionClearWithAlterRegionDUnitTest implements
Serializable {
+
+ @Rule
+ public DistributedRule distributedRule = new DistributedRule();
+
+ @Rule
+ public CacheRule cacheRule = new CacheRule();
+
+ @Rule
+ public ExecutorServiceRule executorServiceRule = new ExecutorServiceRule();
+
+ private VM server1;
+
+ private VM server2;
+
+ private VM server3;
+
+ private static final String REGION_NAME = "testRegion";
+
+ private static final int NUM_ENTRIES = 100;
+
+ private void initialize() {
+server1 = VM.getVM(0);
+server2 = VM.getVM(1);
+
+server1.invoke(() -> {
+ cacheRule.createCache();
+
cacheRule.getCache().createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(REGION_NAME);
+});
+
+server2.invoke(() -> {
+ cacheRule.createCache();
+
cacheRule.getCache().createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(REGION_NAME);
+});
+
+server1.invoke(() -> {
+ populateRegion();
+ Region region = cacheRule.getCache().getRegion(REGION_NAME);
+ assertThat(region.size()).isEqualTo(NUM_ENTRIES);
+});
+
+server2.invoke(() -> {
+ Region region = cacheRule.getCache().getRegion(REGION_NAME);
+ assertThat(region.size()).isEqualTo(NUM_ENTRIES);
+});
+ }
+
+ @Test
+ public void testClearRegionWhileAddingCacheLoader() throws
InterruptedException {
+initialize();
+
+AsyncInvocation asyncInvocation1 = server1.invokeAsync(() -> {
+ cacheRule.getCache().getRegion(REGION_NAME).clear();
+
assertThat(cacheRule.getCache().getRegion(REGION_NAME).size()).isEqualTo(0);
+});
+
+AsyncInvocation asyncInvocation2 = server2.invokeAsync(() -> {
+ alterRegionSetCacheLoader();
+});
+
+asyncInvocation1.await();
+asyncInvocation2.await();
+ }
Review comment:
It is verified at line 104 and line 322.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
jchen21 commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493856692
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
Review comment:
The `addStatisticsToMonitor` method ignores the descriptors other than
`VM_GC_STATS_COLLECTIONS` and `VM_GC_STATS_COLLECTION_TIME`. Can we guarantee
that there is no other descriptor?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493853151
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
+ } else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+collectionTime += value.longValue();
+ }
}
}
@Override
public Number getStatistic(String statName) {
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- return getCollections();
+ return collections;
}
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- return getCollectionTime();
+ return collectionTime;
}
-
return 0;
}
@Override
public void handleNotification(StatisticsNotification notification) {
-decreasePrevValues(statsMap);
-
+// sum up all the count and all the time in the stats included in this
notification
+long totalCount = 0;
+long totalTime = 0;
for (StatisticId statId : notification) {
StatisticDescriptor descriptor = statId.getStatisticDescriptor();
String name = descriptor.getName();
Number value;
-
try {
value = notification.getValue(statId);
} catch (StatisticNotFoundException e) {
value = 0;
}
-
log(name, value);
- increaseStats(name, value);
- statsMap.put(name, value);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+totalCount += value.longValue();
+ }
+
+ else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+totalTime += value.longValue();
+ }
}
+
+collections = totalCount;
+collectionTime = totalTime;
}
Review comment:
Let me correct my terminology a bit… I keep saying "GC type," and that's
not the right distinction. What distinguishes one batch of GC stats from
another is which GarbageCollectorMXBean the values come from. I think (but I'm
not sure) that each GC bean reports about a particular memory space.
So where I say "GC type," a better term might be "memory space".
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493844666
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
+ } else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+collectionTime += value.longValue();
+ }
}
}
@Override
public Number getStatistic(String statName) {
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- return getCollections();
+ return collections;
}
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- return getCollectionTime();
+ return collectionTime;
}
-
return 0;
}
@Override
public void handleNotification(StatisticsNotification notification) {
-decreasePrevValues(statsMap);
-
+// sum up all the count and all the time in the stats included in this
notification
+long totalCount = 0;
+long totalTime = 0;
for (StatisticId statId : notification) {
StatisticDescriptor descriptor = statId.getStatisticDescriptor();
String name = descriptor.getName();
Number value;
-
try {
value = notification.getValue(statId);
} catch (StatisticNotFoundException e) {
value = 0;
}
-
log(name, value);
- increaseStats(name, value);
- statsMap.put(name, value);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+totalCount += value.longValue();
+ }
+
+ else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+totalTime += value.longValue();
+ }
}
+
+collections = totalCount;
+collectionTime = totalTime;
}
Review comment:
> then what would those count/time mean though. It's not right to report
just a single one of them. It's not right to sum them up either then
The stats notification stuff is specifically designed to report only
changes. As a result, old gen and new gen CGs are almost always reported
independently (unless they both happen to change during a single sample).
I think that's why the old code was doing the weird stuff. It was trying to
track the previous value for each kind of GC, so that when a new value arrived,
it could add in just the change in that kind of GC. Decrementing and
incrementing was a puzzling way to do that, but that was the goal.
Here's a way that seems simpler to me than decrementing/incrementing. I'll
use collections as an example, but it's the same for collectionTime.
- Create a map that records the latest value for each GC type.
- Create a field that records the sum of values for all types. This is what
gets reported via getCollections()
- When a new kind of GC is added via addStatisticsToMonitor()…
1. Store the initial value for that GC type in the map.
2. Sum the values of all GC types in the map, and store the sum in the
field.
- When a notification arrives…
1. Store the new value for that GC type in the map.
2. Sum the values of all GC types in the map, and store the sum in the
field.
[GitHub] [geode] jinmeiliao commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
jinmeiliao commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493822296
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
+ } else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+collectionTime += value.longValue();
+ }
}
}
@Override
public Number getStatistic(String statName) {
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- return getCollections();
+ return collections;
}
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- return getCollectionTime();
+ return collectionTime;
}
-
return 0;
}
@Override
public void handleNotification(StatisticsNotification notification) {
-decreasePrevValues(statsMap);
-
+// sum up all the count and all the time in the stats included in this
notification
+long totalCount = 0;
+long totalTime = 0;
for (StatisticId statId : notification) {
StatisticDescriptor descriptor = statId.getStatisticDescriptor();
String name = descriptor.getName();
Number value;
-
try {
value = notification.getValue(statId);
} catch (StatisticNotFoundException e) {
value = 0;
}
-
log(name, value);
- increaseStats(name, value);
- statsMap.put(name, value);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+totalCount += value.longValue();
+ }
+
+ else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+totalTime += value.longValue();
+ }
}
+
+collections = totalCount;
+collectionTime = totalTime;
}
Review comment:
> Please write unit tests for `GCSTatsMonitor` directly, rather than
testing only through `MemberLevelStatsTest`.
the unit test will be just like the MemberLevelStatsTest, only by mocking
all those variables.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
jinmeiliao commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493821071
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
+ } else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+collectionTime += value.longValue();
+ }
}
}
@Override
public Number getStatistic(String statName) {
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- return getCollections();
+ return collections;
}
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- return getCollectionTime();
+ return collectionTime;
}
-
return 0;
}
@Override
public void handleNotification(StatisticsNotification notification) {
-decreasePrevValues(statsMap);
-
+// sum up all the count and all the time in the stats included in this
notification
+long totalCount = 0;
+long totalTime = 0;
for (StatisticId statId : notification) {
StatisticDescriptor descriptor = statId.getStatisticDescriptor();
String name = descriptor.getName();
Number value;
-
try {
value = notification.getValue(statId);
} catch (StatisticNotFoundException e) {
value = 0;
}
-
log(name, value);
- increaseStats(name, value);
- statsMap.put(name, value);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+totalCount += value.longValue();
+ }
+
+ else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+totalTime += value.longValue();
+ }
}
+
+collections = totalCount;
+collectionTime = totalTime;
}
Review comment:
> I think a notification includes only those stats whose values have
changed. If that's correct, what we're summing here is only the _changed_ GC
stats, not _all_ of the GC stats. This will work only if we're absolutely
certain that every stat will change on every sample.
>
> See `ValueMonitor.monitorStatistics()`, which builds the notification only
from updated stats.
>
> See `SampleCollector.sample()`, which adds a stats to the updated stats
list only if the value of the stat changed from the previous sample.
then what would those count/time mean though. It's not right to report just
a single one of them. It's not right to sum them up either then
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] jinmeiliao opened a new pull request #5547: GEODE-8496: fix rest management test after dependency bump
jinmeiliao opened a new pull request #5547: URL: https://github.com/apache/geode/pull/5547 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 d...@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: us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #645: GEODE-8469: Enable no-missing-variable-declarations
pdxcodemonkey commented on pull request #645: URL: https://github.com/apache/geode-native/pull/645#issuecomment-697875154 I think we can diagnose the test problem with a couple of (very tedious) hours of debugging, which is a lot better than rewriting the test. I think we're much closer to tackling the job of rewriting _all_ of the old tests, tho. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] nabarunnag merged pull request #5546: GEODE-8523: Session state document update
nabarunnag merged pull request #5546: URL: https://github.com/apache/geode/pull/5546 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #5546: GEODE-8523: Session state document update
jhuynh1 commented on pull request #5546: URL: https://github.com/apache/geode/pull/5546#issuecomment-697809643 This might have to be backported to previous releases based on when these jars were introduced... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] nabarunnag opened a new pull request #5546: GEODE-8523: Session state document update
nabarunnag opened a new pull request #5546: URL: https://github.com/apache/geode/pull/5546 * geode-tcp and geode-membership added to the list * removed geode-json as it does not exist anymore. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
demery-pivotal commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493761722
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
+ } else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+collectionTime += value.longValue();
+ }
}
}
@Override
public Number getStatistic(String statName) {
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- return getCollections();
+ return collections;
}
if (statName.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- return getCollectionTime();
+ return collectionTime;
}
-
return 0;
}
@Override
public void handleNotification(StatisticsNotification notification) {
-decreasePrevValues(statsMap);
-
+// sum up all the count and all the time in the stats included in this
notification
+long totalCount = 0;
+long totalTime = 0;
for (StatisticId statId : notification) {
StatisticDescriptor descriptor = statId.getStatisticDescriptor();
String name = descriptor.getName();
Number value;
-
try {
value = notification.getValue(statId);
} catch (StatisticNotFoundException e) {
value = 0;
}
-
log(name, value);
- increaseStats(name, value);
- statsMap.put(name, value);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+totalCount += value.longValue();
+ }
+
+ else if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
+totalTime += value.longValue();
+ }
}
+
+collections = totalCount;
+collectionTime = totalTime;
}
Review comment:
I think a notification includes only those stats whose values have
changed. If that's correct, what we're summing here is only the _changed_ GC
stats, not _all_ of the GC stats. This will work only if we're absolutely
certain that every stat will change on every sample.
See `ValueMonitor.monitorStatistics()`, which builds the notification only
from updated stats.
See `SampleCollector.sample()`, which adds a stats to the updated stats list
only if the value of the stat changed from the previous sample.
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
[GitHub] [geode-native] moleske commented on pull request #645: GEODE-8469: Enable no-missing-variable-declarations
moleske commented on pull request #645: URL: https://github.com/apache/geode-native/pull/645#issuecomment-697695288 @pdxcodemonkey do you think it will be easier to rewrite those tests in the new framework rather than find and fix the problem? It may take more time but long term be more valuable This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] upthewaterspout opened a new pull request #5545: GEODE-8522: Switching a exception log back to debug
upthewaterspout opened a new pull request #5545: URL: https://github.com/apache/geode/pull/5545 This log message happens during the course of normal startup of multiple locators. We should not be logging a full stack trace during normal startup. 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 d...@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: us...@infra.apache.org
[GitHub] [geode] sabbeyPivotal opened a new pull request #5544: GEODE-8515: Redis PING should respond appropriately when called from within a SUBSCRIBE/PSUBSCRIBE
sabbeyPivotal opened a new pull request #5544: URL: https://github.com/apache/geode/pull/5544 From PING documentation (https://redis.io/commands/ping): If the client is subscribed to a channel or a pattern, it will instead return a multi-bulk with a "pong" in the first position and an empty bulk in the second position, unless an argument is provided in which case it returns a copy of the argument. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
kirklund commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493738369
##
File path:
geode-core/src/test/java/org/apache/geode/management/bean/stats/MemberLevelStatsTest.java
##
@@ -339,6 +342,41 @@ public void testRegionCounters() {
assertThat(memberMBeanBridge.getTotalPrimaryBucketCount()).isZero();
}
+ @Test
+ public void testVMStats() {
+Statistics[] realStats =
statisticsManager.findStatisticsByType(VMStats50.getGCType());
+long[] totals = modifyStatsAndReturnTotalCountAndTime(10, 2500, realStats);
+memberMBeanBridge.addVMStats(statSampler.getVMStats());
+
assertThat(memberMBeanBridge.getGarbageCollectionCount()).isEqualTo(totals[0]);
+
assertThat(memberMBeanBridge.getGarbageCollectionTime()).isEqualTo(totals[1]);
+
+long[] newTotals = modifyStatsAndReturnTotalCountAndTime(20, 3500,
realStats);
+sampleStats();
+
assertThat(memberMBeanBridge.getGarbageCollectionCount()).isEqualTo(newTotals[0]);
+
assertThat(memberMBeanBridge.getGarbageCollectionTime()).isEqualTo(newTotals[1]);
+ }
+
+ private long[] modifyStatsAndReturnTotalCountAndTime(
+ long baseCount, long baseTime,
+ Statistics[] modifiedStats) {
+long[] totalCountAndTime = {0, 0};
+for (Statistics gcStat : modifiedStats) {
+ StatisticDescriptor[] statistics = gcStat.getType().getStatistics();
+ for (StatisticDescriptor d : statistics) {
+if ("collections".equals(d.getName())) {
Review comment:
I think you should either reference a constant instead of referencing
"collections" and "collectionTime" in multiple classes or go back to using
methods such as getCollections().
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
kirklund commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493739655
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/beans/stats/GCStatsMonitor.java
##
@@ -39,69 +39,71 @@
* @see org.apache.geode.management.internal.beans.stats.MBeanStatsMonitor
*/
public class GCStatsMonitor extends MBeanStatsMonitor {
+ // this class uses these volatile variables to make sure reads are reading
the latest values
+ // it is not using the parent's siteMap which is not volatile to keep the
stats values.
private volatile long collections = 0;
private volatile long collectionTime = 0;
- long getCollections() {
-return collections;
- }
-
- long getCollectionTime() {
-return collectionTime;
- }
-
public GCStatsMonitor(String name) {
super(name);
}
- void decreasePrevValues(Map statsMap) {
-collections -= statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTIONS,
0).longValue();
-collectionTime -=
statsMap.getOrDefault(StatsKey.VM_GC_STATS_COLLECTION_TIME, 0).longValue();
- }
-
- void increaseStats(String name, Number value) {
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
- collections += value.longValue();
- return;
-}
-
-if (name.equals(StatsKey.VM_GC_STATS_COLLECTION_TIME)) {
- collectionTime += value.longValue();
- return;
+ @Override
+ // this will be called multiple times with different collector's stats
+ public void addStatisticsToMonitor(Statistics stats) {
+monitor.addListener(this);// if already listener is added this will be a
no-op
+monitor.addStatistics(stats);
+
+// stats map should keep the sum of all the GC stats
+StatisticDescriptor[] descriptors = stats.getType().getStatistics();
+for (StatisticDescriptor d : descriptors) {
+ String name = d.getName();
+ Number value = stats.get(d);
+ if (name.equals(StatsKey.VM_GC_STATS_COLLECTIONS)) {
+collections += value.longValue();
Review comment:
Using += on a volatile field is not thread safe. I recommend changing
collections and collectionTime to be AtomicLong instances. Then you can use
incrementAndGet() instead of +=. Even if multiple threads do not currently
invoke this method, maybe they will in the future.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on pull request #5538: Bump junit from 4.12 to 4.13
sabbeyPivotal commented on pull request #5538: URL: https://github.com/apache/geode/pull/5538#issuecomment-697674111 I'll take a look at this one! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #5536: GEODE-8520: GCStatsMonitor should sum up all the GC stats to get the …
kirklund commented on a change in pull request #5536:
URL: https://github.com/apache/geode/pull/5536#discussion_r493738369
##
File path:
geode-core/src/test/java/org/apache/geode/management/bean/stats/MemberLevelStatsTest.java
##
@@ -339,6 +342,41 @@ public void testRegionCounters() {
assertThat(memberMBeanBridge.getTotalPrimaryBucketCount()).isZero();
}
+ @Test
+ public void testVMStats() {
+Statistics[] realStats =
statisticsManager.findStatisticsByType(VMStats50.getGCType());
+long[] totals = modifyStatsAndReturnTotalCountAndTime(10, 2500, realStats);
+memberMBeanBridge.addVMStats(statSampler.getVMStats());
+
assertThat(memberMBeanBridge.getGarbageCollectionCount()).isEqualTo(totals[0]);
+
assertThat(memberMBeanBridge.getGarbageCollectionTime()).isEqualTo(totals[1]);
+
+long[] newTotals = modifyStatsAndReturnTotalCountAndTime(20, 3500,
realStats);
+sampleStats();
+
assertThat(memberMBeanBridge.getGarbageCollectionCount()).isEqualTo(newTotals[0]);
+
assertThat(memberMBeanBridge.getGarbageCollectionTime()).isEqualTo(newTotals[1]);
+ }
+
+ private long[] modifyStatsAndReturnTotalCountAndTime(
+ long baseCount, long baseTime,
+ Statistics[] modifiedStats) {
+long[] totalCountAndTime = {0, 0};
+for (Statistics gcStat : modifiedStats) {
+ StatisticDescriptor[] statistics = gcStat.getType().getStatistics();
+ for (StatisticDescriptor d : statistics) {
+if ("collections".equals(d.getName())) {
Review comment:
I think you should either reference a constant instead of referencing
"collections" and "collectionTime" in multiple classes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #5535: GEODE-8516: Add Redis tests for multiple subscriptions for the same client
dschneider-pivotal merged pull request #5535: URL: https://github.com/apache/geode/pull/5535 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #654: GEODE-8514: python protocol tool
pdxcodemonkey commented on pull request #654: URL: https://github.com/apache/geode-native/pull/654#issuecomment-697629814 > @pdxcodemonkey thanks for contributing this! It's also a good excuse to refresh my Python knowledge thanks, @alb3rtobr, hope you find it helpful. Just FYI, I use Black for formatting the Python code, but I don't have it configured in Travis CI or anything, so I'd appreciate if you can use it when contributing any changes. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on pull request #654: GEODE-8514: python protocol tool
alb3rtobr commented on pull request #654: URL: https://github.com/apache/geode-native/pull/654#issuecomment-697558085 @pdxcodemonkey thanks for contributing this! It's also a good excuse to refresh my Python knowledge This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5516: GEODE-7679 Partitioned Region clear is successful while region is being altered
jinmeiliao commented on a change in pull request #5516:
URL: https://github.com/apache/geode/pull/5516#discussion_r493631271
##
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithAlterRegionDUnitTest.java
##
@@ -0,0 +1,424 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.Serializable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.stream.IntStream;
+
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.AttributesMutator;
+import org.apache.geode.cache.CacheListener;
+import org.apache.geode.cache.CacheLoader;
+import org.apache.geode.cache.CacheLoaderException;
+import org.apache.geode.cache.CacheWriter;
+import org.apache.geode.cache.CacheWriterException;
+import org.apache.geode.cache.EntryEvent;
+import org.apache.geode.cache.ExpirationAction;
+import org.apache.geode.cache.ExpirationAttributes;
+import org.apache.geode.cache.LoaderHelper;
+import org.apache.geode.cache.PartitionedRegionPartialClearException;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.distributed.internal.DistributionMessageObserver;
+import org.apache.geode.test.dunit.AsyncInvocation;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class PartitionedRegionClearWithAlterRegionDUnitTest implements
Serializable {
+
+ @Rule
+ public DistributedRule distributedRule = new DistributedRule();
+
+ @Rule
+ public CacheRule cacheRule = new CacheRule();
+
+ @Rule
+ public ExecutorServiceRule executorServiceRule = new ExecutorServiceRule();
+
+ private VM server1;
+
+ private VM server2;
+
+ private VM server3;
+
+ private static final String REGION_NAME = "testRegion";
+
+ private static final int NUM_ENTRIES = 100;
+
+ private void initialize() {
+server1 = VM.getVM(0);
+server2 = VM.getVM(1);
+
+server1.invoke(() -> {
+ cacheRule.createCache();
+
cacheRule.getCache().createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(REGION_NAME);
+});
+
+server2.invoke(() -> {
+ cacheRule.createCache();
+
cacheRule.getCache().createRegionFactory(RegionShortcut.PARTITION).setStatisticsEnabled(true)
+ .create(REGION_NAME);
+});
+
+server1.invoke(() -> {
+ populateRegion();
+ Region region = cacheRule.getCache().getRegion(REGION_NAME);
+ assertThat(region.size()).isEqualTo(NUM_ENTRIES);
+});
+
+server2.invoke(() -> {
+ Region region = cacheRule.getCache().getRegion(REGION_NAME);
+ assertThat(region.size()).isEqualTo(NUM_ENTRIES);
+});
+ }
+
+ @Test
+ public void testClearRegionWhileAddingCacheLoader() throws
InterruptedException {
+initialize();
+
+AsyncInvocation asyncInvocation1 = server1.invokeAsync(() -> {
+ cacheRule.getCache().getRegion(REGION_NAME).clear();
+
assertThat(cacheRule.getCache().getRegion(REGION_NAME).size()).isEqualTo(0);
+});
+
+AsyncInvocation asyncInvocation2 = server2.invokeAsync(() -> {
+ alterRegionSetCacheLoader();
+});
+
+asyncInvocation1.await();
+asyncInvocation2.await();
+ }
Review comment:
do you need to add some verification code to verify that region is
cleared and altered?
##
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithAlterRegionDUnitTest.java
##
@@ -0,0 +1,424 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ *
