jdeppe-pivotal commented on a change in pull request #7478:
URL: https://github.com/apache/geode/pull/7478#discussion_r840737832
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisService.java
##########
@@ -83,18 +86,12 @@ public void handleEvent(ResourceEvent event, Object
resource) {
}
private void startRedisServer(InternalCache cache) {
- InternalDistributedSystem system = cache.getInternalDistributedSystem();
-
- if (system.getConfig().getRedisEnabled()) {
- int port = system.getConfig().getRedisPort();
- String bindAddress = system.getConfig().getRedisBindAddress();
- assert bindAddress != null;
-
- logger.info(
- String.format("Starting GeodeRedisServer on bind address %s on port
%s",
- bindAddress, port));
-
- redisServer = new GeodeRedisServer(bindAddress, port, cache);
+ if (configuration.isEnabled()) {
+ try {
+ redisServer = new GeodeRedisServer(configuration, cache);
+ } catch (IllegalArgumentException ex) {
+ throw new ManagementException(ex);
Review comment:
I think this all ties in to handling a reconnect where the cache is
cycled and services are re-initialized. This happens because
`GeodeRedisService` is a `ResourceEventsListener`. The restart is triggered by
`GemFireCacheImpl.notifyResourceListeners`. In that method various exceptions
are caught and logged, but only `GemFireSecurityException` and
`ManagementException` cause an actual failure. Hence why we wrap in
`ManagementException` here. It's not pretty I know... We probably need to have
`ResourceEventsListener.handleEvent` throw a specific exception if we want the
problem/exception to bubble up.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
