GEODE-8099: make those gfsh commands that updates cluster configurati…

[jinmeiliao]

…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

[agingade]

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() {}

[jinmeiliao]

I believe it is created by this class, see getCmsDlockService in this class

[agingade]

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

[jinmeiliao]

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.

[agingade]

Can that creating (getOrCreateService ) be done in one place, in "LocatorClusterManagementService" which seems to be right place for it...

[jinmeiliao]

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.

[agingade]

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

[jinmeiliao]

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.

[jchen21]

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?

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?

[jinmeiliao]

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.