ISPN-11845 SingleFileStore location validation should be skipped when global state is disabled #8335
Conversation
There was a problem hiding this comment.
@pferraro Thanks for the PR. If we add the check to PersitenceUtil#getLocation then the validation will be applied to all of our file based store implementations.
|
Makes sense. I'll update the PR. |
| Attribute<String> location = attributes.attribute(LOCATION); | ||
| if (location.isNull() && !globalConfig.globalState().enabled()) { | ||
| Log.CONFIG.storeLocationRequired(); | ||
| } |
There was a problem hiding this comment.
We can remove this as the error will always be thrown on startup via PersistenceUtil#getLocation.
There was a problem hiding this comment.
Is it not preferable to detect invalid configuration when the cache configuration is built, rather than fail (after several store startup attempts) when a cache is started?
In WF, this is equivalent to catching the error during a management operation vs failing to deploy an application.
There was a problem hiding this comment.
Yes it is. The correct way is to validate this in the builders, I was trying to save some repetition but this approach makes more sense. The SoftIndexFileStoreConfigurationBuilder will need to be updated to check that the DATA_LOCATION and INDEX_LOCATION in their respective builders are not null. As well the LOCATION and EXPIRATION_LOCATION in the RocksDBStoreConfigurationBuilder.
… global state is disabled
| @@ -2002,4 +2002,7 @@ TimeoutException coordinatorTimeoutWaitingForView(int expectedViewId, int curren | |||
|
|
|||
| @Message(value = "A store cannot be configured with both preload and purgeOnStartup", id = 589) | |||
| CacheConfigurationException preloadAndPurgeOnStartupConflict(); | |||
|
|
|||
| @Message(value = "Store must specify a location when global state is disabled", id = 590) | |||
| CacheConfigurationException storeLocationRequired(); | |||
There was a problem hiding this comment.
To allow for better validation of other store location attributes, we should also create an overloaded version of this message.
@Message(value = "Store must specify the '%s' attribute when global state is disabled", id = 590)
CacheConfigurationException storeLocationRequired(String attributeName);
|
There are lots of related test failures due to the change of behaviour. |
|
Closing in favour of #8361 |
https://issues.redhat.com/browse/ISPN-11845
Please cherry-pick into 10.1.x.