Add option for ISPN cache removal commands to add the allow-resource-… #156
Conversation
| } | ||
|
|
||
| @Override | ||
| public void apply(OnlineCommandContext ctx) throws Exception { | ||
| Operations ops = new Operations(ctx.client); | ||
| if(allowResourceServiceRestart) | ||
| ops = ops.headers(Values.of("allow-resource-service-restart", "true")); |
There was a problem hiding this comment.
Cool, didn't notice. Will use that.
| this(cacheContainer, cacheType, cacheName, false); | ||
| } | ||
|
|
||
| public RemoveCache(String cacheContainer, CacheType cacheType, String cacheName, boolean allowResourceServiceRestart) { |
There was a problem hiding this comment.
We don't have a builder for this class yet, right? Bollean arguments are always a good sign that a builder would be a good idea.
There was a problem hiding this comment.
Yeah but in this case it sounds a bit of an overkill to me, there are 3 required arguments and one optional, builders are suited for classes with a prevalence of optional arguments :) also for keeping backward compatibility we would need to keep the constructor as well, where it gets even weirder. But if you insist, I can do that...
There was a problem hiding this comment.
Yea I see where you're coming from, but the builder can have a constructor with the same 3 arguments and just 1 optional method, so not that big of an overkill IMHO.
There was a problem hiding this comment.
Or possibly this could be automatic, if we detect the server isn't able to remove a cache without service restart (version check).
I know this goes against "restarts should be explicit", but it's an option worth considering. WDYT?
There was a problem hiding this comment.
Version check sounds good, it will be a bit more difficult to remove a cache without automatically calling service restart, but who does that anyway?
There was a problem hiding this comment.
Just please add a comment to the class javadoc that with EAP 7.1+, this command enables the service restart header. That would be good enough from my POV. @rhatlapa, WDYT?
There was a problem hiding this comment.
Shouldn't we try to have the javadoc upstream related => rather pointing out WildFly releases? Otherwise I am OK with both approaches (Builder one and constructor one with one optional argument.
Regarding having it based on Version seems probably better as user usually just wants to remove the cache and have it working on all EAP versions and not need to modify command attributes based on EAP version it is used with.
There was a problem hiding this comment.
Yea I agree, I always try to use upstream names and versions, unless there's something specific to EAP. So this is only WildFly 11+, I think? Which should be ServerVersion.VERSION_5_0_0.
| @@ -17,6 +17,7 @@ | |||
|
|
|||
| import java.io.IOException; | |||
| import java.util.UUID; | |||
| import java.util.concurrent.TimeoutException; | |||
There was a problem hiding this comment.
Oh and I wonder, how come this is now necessary? The apply method signature didn't change, right?
There was a problem hiding this comment.
Right, that's a leftover from my feeble previous attempt where I called reload, I'll get rid of that.
There was a problem hiding this comment.
Cool, that sounds like this will be a tiny PR in the end, I love those :-)
jmartisk
force-pushed
the
ispn-caches-reload
branch
from
62ab875 to
388f132
Compare
| import org.wildfly.extras.creaper.core.online.operations.Operations; | ||
|
|
||
| /** | ||
| * Removes an Infinispan cache. | ||
| * Since WildFly 11 / EAP 7, this operation requires a reload, and this command |
jmartisk
force-pushed
the
ispn-caches-reload
branch
from
388f132 to
e0e6b17
Compare
…ving Infinispan caches on WF11+ and EAP7.1+
|
Thanks! |
…service-restart header
https://issues.jboss.org/browse/JBEAP-8924 was rejected, as expected 😢 🎃