[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17146744#comment-17146744
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r446474804
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
Yes, the `InternalConfigurationPersistenceService` is used by both rest
and gfsh to persist the configuration change in the region, and it already has
its own dlockService to make sure update to the region is thread safe.
What are make thread-safe in here is not just "update to the region", it's
the "update to the server sand then update the region", which is a level above
the persistence layer. Both rest and gfsh does this and so both needs the same
dlockService.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17146746#comment-17146746
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r446474804
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
Yes, the `InternalConfigurationPersistenceService` is used by both rest
and gfsh to persist the configuration change in the region, and it already has
its own dlockService to make sure update to the region is thread safe.
What are trying to. make thread-safe in here is not just "update to the
region", it's the "update to the servers and then update the region", which is
a level above the persistence layer. Both rest and gfsh does this and so both
needs the same dlockService.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17146745#comment-17146745
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r446474804
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
Yes, the `InternalConfigurationPersistenceService` is used by both rest
and gfsh to persist the configuration change in the region, and it already has
its own dlockService to make sure update to the region is thread safe.
What are trying to. make thread-safe in here is not just "update to the
region", it's the "update to the server sand then update the region", which is
a level above the persistence layer. Both rest and gfsh does this and so both
needs the same dlockService.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17145892#comment-17145892
]
ASF GitHub Bot commented on GEODE-8099:
---
agingade commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445867726
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
My understanding was, we have centralized configuration-management
class, that is used by both gfsh and rest end-point to make the configuration
changes, and I thought it was LocatorClusterManagementService, I guess I am
wrong...
Is it "InternalConfigurationPersistenceService" where both gfsh and
end-point make the configuration changes...If so, can the dlock creation and
unlocking/unlocking helper method be added there.
I am trying to see if we can have the lock creation in one place, rather
than in many clients/apis that make the configuration 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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[ https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17145804#comment-17145804 ] ASF GitHub Bot commented on GEODE-8099: --- jinmeiliao merged pull request #5285: URL: https://github.com/apache/geode/pull/5285 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 > Make ClusterConfiguration Service thread safe > - > > Key: GEODE-8099 > URL: https://issues.apache.org/jira/browse/GEODE-8099 > Project: Geode > Issue Type: Bug > Components: configuration >Reporter: Anilkumar Gingade >Priority: Major > Labels: GeodeOperationAPI > Fix For: 1.14.0 > > > When multiple cluster configuration clients (multiple rest clients) connects > and issue configuration commands, the cluster configuration should handle > concurrent request and modify/save/execute the configuration definition in > thread safe manner. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[ https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17145805#comment-17145805 ] ASF subversion and git services commented on GEODE-8099: Commit b65af0ec91c4b79e8a31cdb616b53d6063ef9a7d in geode's branch refs/heads/develop from Jinmei Liao [ https://gitbox.apache.org/repos/asf?p=geode.git;h=b65af0e ] GEODE-8099: make those gfsh commands that updates cluster configurati… (#5285) * command executor will acquire the lock when executing commands that affects cluster configuration. * clean up commands that doesn't need to extends implement SingleGfshCommand * SingleGfshCommand are for those commands that need to update cluster configuration > Make ClusterConfiguration Service thread safe > - > > Key: GEODE-8099 > URL: https://issues.apache.org/jira/browse/GEODE-8099 > Project: Geode > Issue Type: Bug > Components: configuration >Reporter: Anilkumar Gingade >Priority: Major > Labels: GeodeOperationAPI > Fix For: 1.14.0 > > > When multiple cluster configuration clients (multiple rest clients) connects > and issue configuration commands, the cluster configuration should handle > concurrent request and modify/save/execute the configuration definition in > thread safe manner. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17144470#comment-17144470
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445216158
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
Then OnlineCommandProcessor needs to get ahold of
`LocatorClusterManagementService` and get the dLock service from there? What if
CMS is not started? Gfsh still needs to have this dLockService.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17144397#comment-17144397
]
ASF GitHub Bot commented on GEODE-8099:
---
agingade commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445173321
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
Can that creating (getOrCreateService ) be done in one place, in
"LocatorClusterManagementService" which seems to be right place for it...
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17144291#comment-17144291
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445142798
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/cli/SingleGfshCommand.java
##
@@ -39,7 +42,11 @@
*return value of your command method.
* @return a boolean indicating whether a change to the cluster
configuration was persisted.
*/
- public boolean updateConfigForGroup(String group, CacheConfig config, Object
configObject) {
-return false;
+ public abstract boolean updateConfigForGroup(String group, CacheConfig
config,
Review comment:
this class is @experimental
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17144289#comment-17144289
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445142428
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java
##
@@ -53,14 +57,13 @@
private SecurityService securityService;
- private InternalCache cache;
-
public OnlineCommandProcessor() {}
@VisibleForTesting
- public OnlineCommandProcessor(Properties cacheProperties, SecurityService
securityService,
- CommandExecutor commandExecutor, InternalCache cache) {
-this.gfshParser = new GfshParser(new CommandManager(cacheProperties,
cache));
+ public OnlineCommandProcessor(GfshParser gfshParser,
Review comment:
the change is necessary for the test to pass. All this change is because
public methods in DlockService are static.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17144287#comment-17144287
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445141795
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/cli/SingleGfshCommand.java
##
@@ -39,7 +42,11 @@
*return value of your command method.
* @return a boolean indicating whether a change to the cluster
configuration was persisted.
*/
- public boolean updateConfigForGroup(String group, CacheConfig config, Object
configObject) {
-return false;
+ public abstract boolean updateConfigForGroup(String group, CacheConfig
config,
+ Object configObject);
+
+ @Override
+ public boolean affectsClusterConfiguration() {
Review comment:
It's marked as @Experimental
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[ https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17144255#comment-17144255 ] ASF GitHub Bot commented on GEODE-8099: --- jinmeiliao commented on pull request #5285: URL: https://github.com/apache/geode/pull/5285#issuecomment-649039263 > I have some questions about `SingleGfshCommand`, which is extended by commands that will update cluster config. Now in this pull request, quite a few classes used to be subclasses of `SingleGfshCommand` now become the subclasses of `GfshCommand`. Does it mean that these classes don't update cluster config any more? They don't update cluster config at all. They don't need to extends SingleGfshCommand in the first place and they didn't even implement the `updateGroupConfig` method at all. > > By default, the subclasses of `GfshCommand` returns `false` for `affectsClusterConfiguration`. But some class, e.g. `AlterRuntimeConfigCommand` that extends `GfshCommand` returns `true` for `affectsClusterConfiguration`. Why is that? Because AlterRuntimConfigComand does affect cluster configuration. It updates the properties part of the cluster configuration. 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 > Make ClusterConfiguration Service thread safe > - > > Key: GEODE-8099 > URL: https://issues.apache.org/jira/browse/GEODE-8099 > Project: Geode > Issue Type: Bug > Components: configuration >Reporter: Anilkumar Gingade >Priority: Major > Labels: GeodeOperationAPI > Fix For: 1.14.0 > > > When multiple cluster configuration clients (multiple rest clients) connects > and issue configuration commands, the cluster configuration should handle > concurrent request and modify/save/execute the configuration definition in > thread safe manner. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17144253#comment-17144253
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445136917
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
they both call `DLockService.getOrCreateService`, if it's already
created, it will just get it. So, I guess whoever get to it first will create
it.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17144222#comment-17144222
]
ASF GitHub Bot commented on GEODE-8099:
---
jchen21 commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445095363
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/cli/SingleGfshCommand.java
##
@@ -39,7 +42,11 @@
*return value of your command method.
* @return a boolean indicating whether a change to the cluster
configuration was persisted.
*/
- public boolean updateConfigForGroup(String group, CacheConfig config, Object
configObject) {
-return false;
+ public abstract boolean updateConfigForGroup(String group, CacheConfig
config,
Review comment:
This changes the public API, which could affect those gfsh commands that
extend this class.
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java
##
@@ -53,14 +57,13 @@
private SecurityService securityService;
- private InternalCache cache;
-
public OnlineCommandProcessor() {}
@VisibleForTesting
- public OnlineCommandProcessor(Properties cacheProperties, SecurityService
securityService,
- CommandExecutor commandExecutor, InternalCache cache) {
-this.gfshParser = new GfshParser(new CommandManager(cacheProperties,
cache));
+ public OnlineCommandProcessor(GfshParser gfshParser,
Review comment:
This constructor is used only by tests. e.g. `MemberCommandServiceTest`.
This also leads to adding a new constructor to a deprecated class
`MemberCommandService`. And then changes to `MemberCommandServiceTest`. I
wonder if it is necessary to make this change.
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/cli/SingleGfshCommand.java
##
@@ -39,7 +42,11 @@
*return value of your command method.
* @return a boolean indicating whether a change to the cluster
configuration was persisted.
*/
- public boolean updateConfigForGroup(String group, CacheConfig config, Object
configObject) {
-return false;
+ public abstract boolean updateConfigForGroup(String group, CacheConfig
config,
+ Object configObject);
+
+ @Override
+ public boolean affectsClusterConfiguration() {
Review comment:
This changes the public API, which could affect those gfsh commands that
extend this class.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17144193#comment-17144193
]
ASF GitHub Bot commented on GEODE-8099:
---
agingade commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445095963
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
In this PR. the Dlock is getting created in
"OnlineCommandProcessor.java"; I don't have latest develop, my guess it may be
getting created in rest endpoint too...
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17143481#comment-17143481
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r444618488
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
I believe it is created by this class, see `getCmsDlockService` in this
class
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17143480#comment-17143480
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r444618066
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/remote/CommandExecutor.java
##
@@ -207,4 +219,32 @@ protected Object invokeCommand(Object command,
GfshParseResult parseResult) {
return resultModel;
}
+
+ @VisibleForTesting
+ boolean lockCMS(Object command) {
+if (cmsDlockService == null) {
+ return false;
+}
+if (!(command instanceof GfshCommand)) {
Review comment:
I think this reads better and I plan to add comments in each if block
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17143434#comment-17143434
]
ASF GitHub Bot commented on GEODE-8099:
---
agingade commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r444588637
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -113,7 +113,7 @@
public class LocatorClusterManagementService implements
ClusterManagementService {
@VisibleForTesting
// the dlock service name used by the CMS
- static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
+ public static final String CMS_DLOCK_SERVICE_NAME = "CMS_DLOCK_SERVICE";
Review comment:
Since "LocatorClusterManagementService" is central to the
ClusterConfigurationService; will it be better to have the cms-dlock service
created in this class; and have helper method to lock and unlock. Assumption is
when the lock is requested the Locator Management service will be up and
running.
lockConfigForUpdate() {}
unLockConfigForUpdate() {}
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/cli/GfshCommand.java
##
@@ -59,6 +59,14 @@ public boolean isOnlineCommandAvailable() {
return gfsh.isConnectedAndReady();
}
+ /**
+ * For those commands that could possibly change the cluster configuration,
they need to
+ * override this method to return true.
+ */
+ public boolean affectsClusterConfiguration() {
Review comment:
How about naming it as "updatesClusterConfiguration" (); they both are
same, just another name to consider, its left to you.
##
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/remote/CommandExecutor.java
##
@@ -207,4 +219,32 @@ protected Object invokeCommand(Object command,
GfshParseResult parseResult) {
return resultModel;
}
+
+ @VisibleForTesting
+ boolean lockCMS(Object command) {
+if (cmsDlockService == null) {
+ return false;
+}
+if (!(command instanceof GfshCommand)) {
Review comment:
How about combining all of the if into an AND condition.
if (cmsDlockService == null && !(command instanceof GfshCommand) &&...)
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[ https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17142540#comment-17142540 ] ASF GitHub Bot commented on GEODE-8099: --- jinmeiliao opened a new pull request #5285: URL: https://github.com/apache/geode/pull/5285 …on thead safe. * command executor will acquire the lock when executing commands that affects cluster configuration. * clean up commands that doesn't need to extends implement SingleGfshCommand * SingleGfshCommand are for those commands that need to update cluster configuration 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 > Make ClusterConfiguration Service thread safe > - > > Key: GEODE-8099 > URL: https://issues.apache.org/jira/browse/GEODE-8099 > Project: Geode > Issue Type: Bug > Components: configuration >Reporter: Anilkumar Gingade >Priority: Major > Labels: GeodeOperationAPI > Fix For: 1.14.0 > > > When multiple cluster configuration clients (multiple rest clients) connects > and issue configuration commands, the cluster configuration should handle > concurrent request and modify/save/execute the configuration definition in > thread safe manner. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[ https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17136208#comment-17136208 ] ASF subversion and git services commented on GEODE-8099: Commit 0f763eadfbbe4ce25c0628370199ba0850f6d630 in geode's branch refs/heads/develop from Jinmei Liao [ https://gitbox.apache.org/repos/asf?p=geode.git;h=0f763ea ] GEODE-8099: add dlock around cms create/delete operations. (#5188) > Make ClusterConfiguration Service thread safe > - > > Key: GEODE-8099 > URL: https://issues.apache.org/jira/browse/GEODE-8099 > Project: Geode > Issue Type: Bug > Components: configuration >Reporter: Anilkumar Gingade >Priority: Major > Labels: GeodeOperationAPI > Fix For: 1.14.0 > > > When multiple cluster configuration clients (multiple rest clients) connects > and issue configuration commands, the cluster configuration should handle > concurrent request and modify/save/execute the configuration definition in > thread safe manner. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[ https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17136207#comment-17136207 ] ASF GitHub Bot commented on GEODE-8099: --- jinmeiliao merged pull request #5188: URL: https://github.com/apache/geode/pull/5188 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 > Make ClusterConfiguration Service thread safe > - > > Key: GEODE-8099 > URL: https://issues.apache.org/jira/browse/GEODE-8099 > Project: Geode > Issue Type: Bug > Components: configuration >Reporter: Anilkumar Gingade >Priority: Major > Labels: GeodeOperationAPI > Fix For: 1.14.0 > > > When multiple cluster configuration clients (multiple rest clients) connects > and issue configuration commands, the cluster configuration should handle > concurrent request and modify/save/execute the configuration definition in > thread safe manner. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17136021#comment-17136021
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r440316670
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
##
@@ -125,16 +127,41 @@ public void before() throws Exception {
doReturn(true).when(persistenceService).lockSharedConfiguration();
doNothing().when(persistenceService).unlockSharedConfiguration();
operationManager = mock(OperationManager.class);
+dLockService = mock(DistributedLockService.class);
+
service =
spy(new LocatorClusterManagementService(cache, persistenceService,
managers, validators,
memberValidator, cacheElementValidator, operationManager));
+doReturn(dLockService).when(service).getCmsDlockService();
regionConfig = new Region();
regionConfig.setName("region1");
rebalanceOperation = new RebalanceOperation();
}
+ @Test
+ public void lockAndUnlockCalledAtCreate() {
+try {
+ service.create(regionConfig);
+} catch (Exception ignore) {
Review comment:
same as "delete" case.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17136019#comment-17136019
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r440316375
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -154,6 +163,24 @@ public LocatorClusterManagementService(
this.operationManager = operationManager;
}
+ @VisibleForTesting
+ DistributedLockService getCmsDlockService() {
Review comment:
good catch.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17136018#comment-17136018
]
ASF GitHub Bot commented on GEODE-8099:
---
jinmeiliao commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r440316264
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
##
@@ -125,16 +127,41 @@ public void before() throws Exception {
doReturn(true).when(persistenceService).lockSharedConfiguration();
doNothing().when(persistenceService).unlockSharedConfiguration();
operationManager = mock(OperationManager.class);
+dLockService = mock(DistributedLockService.class);
+
service =
spy(new LocatorClusterManagementService(cache, persistenceService,
managers, validators,
memberValidator, cacheElementValidator, operationManager));
+doReturn(dLockService).when(service).getCmsDlockService();
regionConfig = new Region();
regionConfig.setName("region1");
rebalanceOperation = new RebalanceOperation();
}
+ @Test
+ public void lockAndUnlockCalledAtCreate() {
+try {
+ service.create(regionConfig);
+} catch (Exception ignore) {
+}
+
+
verify(dLockService).lock(LocatorClusterManagementService.CMS_DLOCK_SERVICE_NAME,
-1, -1);
+
verify(dLockService).unlock(LocatorClusterManagementService.CMS_DLOCK_SERVICE_NAME);
+ }
+
+ @Test
+ public void lockAndUnlockCalledAtDelete() {
+try {
+ service.delete(regionConfig);
+} catch (Exception ignore) {
Review comment:
in this test setup, a NPE will already be thrown. This test is to make
sure the two calls always get called when any exception happens. I changed the
test method to be `lockAndUnlockCalledAtDeleteWithException`. And added the
verification to another test setup when deletion is successful.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17135984#comment-17135984
]
ASF GitHub Bot commented on GEODE-8099:
---
kirklund commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r440286551
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
##
@@ -125,16 +127,41 @@ public void before() throws Exception {
doReturn(true).when(persistenceService).lockSharedConfiguration();
doNothing().when(persistenceService).unlockSharedConfiguration();
operationManager = mock(OperationManager.class);
+dLockService = mock(DistributedLockService.class);
+
service =
spy(new LocatorClusterManagementService(cache, persistenceService,
managers, validators,
memberValidator, cacheElementValidator, operationManager));
+doReturn(dLockService).when(service).getCmsDlockService();
regionConfig = new Region();
regionConfig.setName("region1");
rebalanceOperation = new RebalanceOperation();
}
+ @Test
+ public void lockAndUnlockCalledAtCreate() {
+try {
+ service.create(regionConfig);
+} catch (Exception ignore) {
Review comment:
Is create expected to throw some Exception every time this test runs? If
not, then let's remove the try-catch and add `throws Exception` clause to the
method.
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
##
@@ -125,16 +127,41 @@ public void before() throws Exception {
doReturn(true).when(persistenceService).lockSharedConfiguration();
doNothing().when(persistenceService).unlockSharedConfiguration();
operationManager = mock(OperationManager.class);
+dLockService = mock(DistributedLockService.class);
+
service =
spy(new LocatorClusterManagementService(cache, persistenceService,
managers, validators,
memberValidator, cacheElementValidator, operationManager));
+doReturn(dLockService).when(service).getCmsDlockService();
regionConfig = new Region();
regionConfig.setName("region1");
rebalanceOperation = new RebalanceOperation();
}
+ @Test
+ public void lockAndUnlockCalledAtCreate() {
+try {
+ service.create(regionConfig);
+} catch (Exception ignore) {
+}
+
+
verify(dLockService).lock(LocatorClusterManagementService.CMS_DLOCK_SERVICE_NAME,
-1, -1);
+
verify(dLockService).unlock(LocatorClusterManagementService.CMS_DLOCK_SERVICE_NAME);
+ }
+
+ @Test
+ public void lockAndUnlockCalledAtDelete() {
+try {
+ service.delete(regionConfig);
+} catch (Exception ignore) {
Review comment:
Can we remove try-block and use `throws Exception`?
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -154,6 +163,24 @@ public LocatorClusterManagementService(
this.operationManager = operationManager;
}
+ @VisibleForTesting
+ DistributedLockService getCmsDlockService() {
Review comment:
This method should be `synchronized` since cmsDlockService is lazy
initialized.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17132651#comment-17132651
]
ASF GitHub Bot commented on GEODE-8099:
---
jchen21 commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r438353291
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/BaseManagementService.java
##
@@ -94,6 +94,13 @@ public static void
setManagementService(InternalCacheForClientAccess cache,
}
}
+ @VisibleForTesting
+ public static void clearManagementService(InternalCacheForClientAccess
cache) {
+synchronized (instances) {
+ instances.remove(cache);
Review comment:
Looking at `BaseManagementService`, this is the only `instances.remove`
statement to remove an entry from the `HashMap`. Entries are put to the
`HashMap`, but never removed individually. There is `instances.clear`, but it
removes everything in the `HashMap`. This concern maybe outside the scope of
this JIRA ticket though.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[ https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17129566#comment-17129566 ] ASF GitHub Bot commented on GEODE-8099: --- jinmeiliao commented on a change in pull request #5188: URL: https://github.com/apache/geode/pull/5188#discussion_r437003230 ## File path: geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java ## @@ -114,6 +122,7 @@ private final MemberValidator memberValidator; private final CommonConfigurationValidator commonValidator; private final InternalCache cache; + private DistributedLockService cmsDlockService; Review comment: I tried your approach, but that affects lots of other integration tests that uses some API in InternalLocator that would start the cms service and hit error when they can't get the dlockservice to inject to it. ## File path: geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java ## @@ -114,6 +122,7 @@ private final MemberValidator memberValidator; private final CommonConfigurationValidator commonValidator; private final InternalCache cache; + private DistributedLockService cmsDlockService; Review comment: I tried your approach, but that affects lots of other integration tests that uses some API in InternalLocator that would start the cms service and hit error when they can't get the dlockservice to inject to it. So I had to revert it back 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 > Make ClusterConfiguration Service thread safe > - > > Key: GEODE-8099 > URL: https://issues.apache.org/jira/browse/GEODE-8099 > Project: Geode > Issue Type: Bug > Components: configuration >Reporter: Anilkumar Gingade >Priority: Major > Labels: GeodeOperationAPI > Fix For: 1.14.0 > > > When multiple cluster configuration clients (multiple rest clients) connects > and issue configuration commands, the cluster configuration should handle > concurrent request and modify/save/execute the configuration definition in > thread safe manner. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17127148#comment-17127148
]
ASF GitHub Bot commented on GEODE-8099:
---
mhansonp commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r436208620
##
File path:
geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorClusterManagementServiceIntegrationTest.java
##
@@ -56,13 +57,38 @@ public void tearDown() {
}
}
+ @Before
+ public void setup() throws URISyntaxException {
+distributionConfig = mock(DistributionConfigImpl.class);
+cache = mock(InternalCacheForClientAccess.class);
+managementService = mock(BaseManagementService.class);
Review comment:
With these changes does this test need to become a unit test rather than
an integration test?
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17126995#comment-17126995
]
ASF GitHub Bot commented on GEODE-8099:
---
kirklund commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r436081052
##
File path:
geode-core/src/integrationTest/java/org/apache/geode/distributed/internal/InternalLocatorClusterManagementServiceIntegrationTest.java
##
@@ -56,13 +57,38 @@ public void tearDown() {
}
}
+ @Before
+ public void setup() throws URISyntaxException {
Review comment:
I recommend moving all the field and var creations at the top, then move
all the when blocks together, order both blocks in some way (I alphabetize),
and then consider moving the subject(s) under test to the bottom of the setup
method.
I also recommend cleaning up any statics in tearDown that you've configured
in setUp. For example: how to undo this statement in tearDown:
```
BaseManagementService.setManagementService(cache, managementService);
```
The statics will still have this configured after each test completes.
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -114,6 +122,7 @@
private final MemberValidator memberValidator;
private final CommonConfigurationValidator commonValidator;
private final InternalCache cache;
+ private DistributedLockService cmsDlockService;
Review comment:
Let's move away from lazy-initializing and mutable fields and just make
it final.
I'm assuming you made it mutable for testing. This is where you typically
want to chain more than one constructor. One of the constructors needs
`@VisibleForTesting` and pass in the DistributedLockService (as a mock or
whatever). The main public constructor can then create the
DistributedLockService and pass it as a parameter to the next constructor. The
test would use the next constructor that accepts a DistributedLockService.
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
> Make ClusterConfiguration Service thread safe
> -
>
> Key: GEODE-8099
> URL: https://issues.apache.org/jira/browse/GEODE-8099
> Project: Geode
> Issue Type: Bug
> Components: configuration
>Reporter: Anilkumar Gingade
>Priority: Major
> Labels: GeodeOperationAPI
> Fix For: 1.14.0
>
>
> When multiple cluster configuration clients (multiple rest clients) connects
> and issue configuration commands, the cluster configuration should handle
> concurrent request and modify/save/execute the configuration definition in
> thread safe manner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[
https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17124229#comment-17124229
]
ASF GitHub Bot commented on GEODE-8099:
---
jchen21 commented on a change in pull request #5188:
URL: https://github.com/apache/geode/pull/5188#discussion_r434115982
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -105,7 +107,12 @@
import org.apache.geode.management.runtime.OperationResult;
import org.apache.geode.management.runtime.RuntimeInfo;
+/**
+ * each locator will have one instance of this running if enabled
+ */
public class LocatorClusterManagementService implements
ClusterManagementService {
+ @VisibleForTesting
+ static final String CMS_NAME = "CMS_LOCK_SERVICE";
Review comment:
It would be better to add some comments for this constant.
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
##
@@ -125,22 +127,48 @@ public void before() throws Exception {
doReturn(true).when(persistenceService).lockSharedConfiguration();
doNothing().when(persistenceService).unlockSharedConfiguration();
operationManager = mock(OperationManager.class);
+dLockService = mock(DistributedLockService.class);
+
service =
spy(new LocatorClusterManagementService(cache, persistenceService,
managers, validators,
memberValidator, cacheElementValidator, operationManager));
+doReturn(dLockService).when(service).getCmsDlockService();
regionConfig = new Region();
regionConfig.setName("region1");
rebalanceOperation = new RebalanceOperation();
}
+ @Test
+ public void lockAndUnlockCalledAtCreate() {
+try {
+ service.create(regionConfig);
+} catch (Exception ignore) {
+}
+
+verify(dLockService).lock(LocatorClusterManagementService.CMS_NAME, -1,
-1);
+verify(dLockService).unlock(LocatorClusterManagementService.CMS_NAME);
+ }
+
+ @Test
+ public void lockAndUnlockCalledAtDelete() {
+try {
+ service.delete(regionConfig);
+} catch (Exception ignore) {
+}
+
+verify(dLockService).lock(LocatorClusterManagementService.CMS_NAME, -1,
-1);
+verify(dLockService).unlock(LocatorClusterManagementService.CMS_NAME);
+ }
+
@Test
public void create_persistenceIsNull() {
org.apache.geode.cache.Region region =
mock(org.apache.geode.cache.Region.class);
when(cache.getRegion(any())).thenReturn(region);
-service = new LocatorClusterManagementService(cache, null);
Review comment:
This code change could loose code coverage for the constructor
`LocatorClusterManagementService(InternalCache cache,
InternalConfigurationPersistenceService persistenceService)`. I don't see this
constructor is covered anywhere else.
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
##
@@ -105,7 +107,12 @@
import org.apache.geode.management.runtime.OperationResult;
import org.apache.geode.management.runtime.RuntimeInfo;
+/**
+ * each locator will have one instance of this running if enabled
+ */
public class LocatorClusterManagementService implements
ClusterManagementService {
+ @VisibleForTesting
+ static final String CMS_NAME = "CMS_LOCK_SERVICE";
Review comment:
It would be better to add some comment for this constant.
##
File path:
geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
##
@@ -125,22 +127,48 @@ public void before() throws Exception {
doReturn(true).when(persistenceService).lockSharedConfiguration();
doNothing().when(persistenceService).unlockSharedConfiguration();
operationManager = mock(OperationManager.class);
+dLockService = mock(DistributedLockService.class);
+
service =
spy(new LocatorClusterManagementService(cache, persistenceService,
managers, validators,
memberValidator, cacheElementValidator, operationManager));
+doReturn(dLockService).when(service).getCmsDlockService();
regionConfig = new Region();
regionConfig.setName("region1");
rebalanceOperation = new RebalanceOperation();
}
+ @Test
+ public void lockAndUnlockCalledAtCreate() {
+try {
+ service.create(regionConfig);
+} catch (Exception ignore) {
+}
+
+verify(dLockService).lock(LocatorClusterManagementService.CMS_NAME, -1,
-1);
+verify(dLockService).unlock(LocatorClusterManagementService.CMS_NAME);
+ }
+
+ @Test
+ public void lockAndUnlockCalledAtDelete() {
+try {
+ service.delete(regionConfig);
+} catch (Exception ignore) {
+}
+
+verify(dLockService).lock(LocatorClusterManagementService.CMS_NAME, -1,
-1);
+
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[ https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17120033#comment-17120033 ] ASF GitHub Bot commented on GEODE-8099: --- jinmeiliao opened a new pull request #5188: URL: https://github.com/apache/geode/pull/5188 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 > Make ClusterConfiguration Service thread safe > - > > Key: GEODE-8099 > URL: https://issues.apache.org/jira/browse/GEODE-8099 > Project: Geode > Issue Type: Bug > Components: configuration >Reporter: Anilkumar Gingade >Priority: Major > Labels: GeodeOperationAPI > Fix For: 1.14.0 > > > When multiple cluster configuration clients (multiple rest clients) connects > and issue configuration commands, the cluster configuration should handle > concurrent request and modify/save/execute the configuration definition in > thread safe manner. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8099) Make ClusterConfiguration Service thread safe
[ https://issues.apache.org/jira/browse/GEODE-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17102991#comment-17102991 ] Geode Integration commented on GEODE-8099: -- A Pivotal Tracker story has been created for this Issue: https://www.pivotaltracker.com/story/show/172748710 > Make ClusterConfiguration Service thread safe > - > > Key: GEODE-8099 > URL: https://issues.apache.org/jira/browse/GEODE-8099 > Project: Geode > Issue Type: Bug > Components: configuration >Reporter: Anilkumar Gingade >Priority: Major > Labels: GeodeOperationAPI > Fix For: 1.14.0 > > > When multiple cluster configuration clients (gfsh, rest, etc) connects and > issue configuration commands, the cluster configuration should handle > concurrent request and modify/save/execute the configuration definition in > thread safe manner. -- This message was sent by Atlassian Jira (v8.3.4#803005)
