You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/06/23 02:12:21 UTC
[GitHub] [geode] jinmeiliao opened a new pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
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:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao merged pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org