You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by davinash <gi...@git.apache.org> on 2016/11/29 09:40:46 UTC
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
GitHub user davinash opened a pull request:
https://github.com/apache/incubator-geode/pull/299
[ GEODE-2141 ] #comment Fix Issue #2141
Replaced List with ConcurrentHashSet
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/davinash/incubator-geode feature/GEODE-2141
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-geode/pull/299.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #299
----
commit e4da4c43dc691e59d34640e95c5d63a5a1b0ea1c
Author: adongre <av...@ampool.io>
Date: 2016-11-27T14:11:48Z
GEODE-2141: Replace use of List with ConcurrentHashSet for storing various stats and listener.
Moved methods stopNotifier_IfEnabledAndRunning and startNotifier_IfEnabledAndNotRunning in synchronized
block.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90025338
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsMonitor.java ---
@@ -29,23 +29,20 @@
private final Object mutex = new Object();
- private volatile List<StatisticsListener> listeners = Collections.<StatisticsListener>emptyList();
+ private volatile ConcurrentHashSet<StatisticsListener> listeners =
--- End diff --
Instead of `volatile` I think this should be final. I don't see a write to this var after initialization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode issue #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by upthewaterspout <gi...@git.apache.org>.
Github user upthewaterspout commented on the issue:
https://github.com/apache/incubator-geode/pull/299
+1 - looks good to me, I think it's ready to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90026482
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsMonitor.java ---
@@ -114,14 +100,14 @@ protected void monitor(long millisTimeStamp, List<ResourceInstance> resourceInst
private final void monitorStatisticIds(long millisTimeStamp,
List<ResourceInstance> resourceInstances) {
- List<StatisticId> statisticIdsToMonitor = statisticIds;
+ ConcurrentHashSet<StatisticId> statisticIdsToMonitor = statisticIds;
--- End diff --
I don't think this copy is needed since `statisticIdsToMonitor` is never updated after initialization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90025325
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsMonitor.java ---
@@ -29,23 +29,20 @@
private final Object mutex = new Object();
- private volatile List<StatisticsListener> listeners = Collections.<StatisticsListener>emptyList();
+ private volatile ConcurrentHashSet<StatisticsListener> listeners =
+ new ConcurrentHashSet<StatisticsListener>();
- private volatile List<StatisticId> statisticIds = Collections.<StatisticId>emptyList();
+ private volatile ConcurrentHashSet<StatisticId> statisticIds =
--- End diff --
Instead of `volatile` I think this should be final. I don't see a write to this var after initialization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode issue #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by davinash <gi...@git.apache.org>.
Github user davinash commented on the issue:
https://github.com/apache/incubator-geode/pull/299
Thanks @upthewaterspout , @metatype for review
I Have address review comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90025623
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsMonitor.java ---
@@ -114,14 +100,14 @@ protected void monitor(long millisTimeStamp, List<ResourceInstance> resourceInst
private final void monitorStatisticIds(long millisTimeStamp,
List<ResourceInstance> resourceInstances) {
- List<StatisticId> statisticIdsToMonitor = statisticIds;
+ ConcurrentHashSet<StatisticId> statisticIdsToMonitor = statisticIds;
if (!statisticIdsToMonitor.isEmpty()) {
// TODO:
}
}
protected final void notifyListeners(StatisticsNotification notification) {
- List<StatisticsListener> listenersToNotify = this.listeners;
+ ConcurrentHashSet<StatisticsListener> listenersToNotify = this.listeners;
--- End diff --
I don't think this copy is needed since `listeners` is never updated after initialization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90025856
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java ---
@@ -228,7 +222,7 @@ private void work() {
}
}
if (working && latestTask != null) {
- List<StatisticsMonitor> currentMonitors = StatMonitorHandler.this.monitors;
+ ConcurrentHashSet<StatisticsMonitor> currentMonitors = StatMonitorHandler.this.monitors;
--- End diff --
I don't think this copy is needed since `monitors` is never updated after initialization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by upthewaterspout <gi...@git.apache.org>.
Github user upthewaterspout commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90078053
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java ---
@@ -138,8 +127,8 @@ public void allocatedResourceInstance(ResourceInstance resourceInstance) {}
public void destroyedResourceInstance(ResourceInstance resourceInstance) {}
/** For testing only */
- List<StatisticsMonitor> getMonitorsSnapshot() {
- return Collections.unmodifiableList(this.monitors);
+ ConcurrentHashSet<StatisticsMonitor> getMonitorsSnapshot() {
--- End diff --
Maybe this ought to just return a Collection?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by davinash <gi...@git.apache.org>.
Github user davinash closed the pull request at:
https://github.com/apache/incubator-geode/pull/299
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90025760
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java ---
@@ -110,7 +100,7 @@ public void sampled(long nanosTimeStamp, List<ResourceInstance> resourceInstance
}
private void monitor(final long sampleTimeMillis, final List<ResourceInstance> resourceInstance) {
- List<StatisticsMonitor> currentMonitors = StatMonitorHandler.this.monitors;
+ ConcurrentHashSet<StatisticsMonitor> currentMonitors = StatMonitorHandler.this.monitors;
--- End diff --
I don't think this copy is needed since `monitors` is never updated after initialization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by upthewaterspout <gi...@git.apache.org>.
Github user upthewaterspout commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90077848
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java ---
@@ -50,36 +50,26 @@ public StatMonitorHandler() {
/** Adds a monitor which will be notified of samples */
public boolean addMonitor(StatisticsMonitor monitor) {
- synchronized (this) {
- boolean added = false;
- List<StatisticsMonitor> oldMonitors = this.monitors;
- if (!oldMonitors.contains(monitor)) {
- List<StatisticsMonitor> newMonitors = new ArrayList<StatisticsMonitor>(oldMonitors);
- added = newMonitors.add(monitor);
- this.monitors = Collections.unmodifiableList(newMonitors);
- }
- if (!this.monitors.isEmpty()) {
- startNotifier_IfEnabledAndNotRunning();
- }
- return added;
+ boolean added = false;
+ if (!this.monitors.contains(monitor)) {
+ added = this.monitors.add(monitor);
+ }
+ if (!this.monitors.isEmpty()) {
+ startNotifier_IfEnabledAndNotRunning();
}
+ return added;
}
/** Removes a monitor that will no longer be used */
public boolean removeMonitor(StatisticsMonitor monitor) {
- synchronized (this) {
- boolean removed = false;
- List<StatisticsMonitor> oldMonitors = this.monitors;
- if (oldMonitors.contains(monitor)) {
- List<StatisticsMonitor> newMonitors = new ArrayList<StatisticsMonitor>(oldMonitors);
- removed = newMonitors.remove(monitor);
- this.monitors = Collections.unmodifiableList(newMonitors);
- }
- if (this.monitors.isEmpty()) {
- stopNotifier_IfEnabledAndRunning();
- }
- return removed;
+ boolean removed = false;
--- End diff --
Same issue here - it's not safe to remove the synchronization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by upthewaterspout <gi...@git.apache.org>.
Github user upthewaterspout commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90077733
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java ---
@@ -50,36 +50,26 @@ public StatMonitorHandler() {
/** Adds a monitor which will be notified of samples */
public boolean addMonitor(StatisticsMonitor monitor) {
- synchronized (this) {
- boolean added = false;
- List<StatisticsMonitor> oldMonitors = this.monitors;
- if (!oldMonitors.contains(monitor)) {
- List<StatisticsMonitor> newMonitors = new ArrayList<StatisticsMonitor>(oldMonitors);
- added = newMonitors.add(monitor);
- this.monitors = Collections.unmodifiableList(newMonitors);
- }
- if (!this.monitors.isEmpty()) {
- startNotifier_IfEnabledAndNotRunning();
- }
- return added;
+ boolean added = false;
--- End diff --
I don't think it's safe to remove the synchronization here. You're calling contains followed by add followed by isEmpty. Also, startNotifier_... is dependent on the state of this.monitors. this.monitors could be getting changed by other threads at any point in here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode issue #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by davinash <gi...@git.apache.org>.
Github user davinash commented on the issue:
https://github.com/apache/incubator-geode/pull/299
Is this PR good to merge to develop ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #299: [ GEODE-2141 ] #comment Fix Issue #2141
Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/299#discussion_r90025277
--- Diff: geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java ---
@@ -38,7 +37,8 @@
private final boolean enableMonitorThread;
/** The registered monitors */
- private volatile List<StatisticsMonitor> monitors = Collections.<StatisticsMonitor>emptyList();
+ private volatile ConcurrentHashSet<StatisticsMonitor> monitors =
--- End diff --
Instead of `volatile` I think this should be final. I don't see a write to this var after initialization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---