[jboss-as7-dev] Management proposal for attribute mutability

[Brian Stansberry]

On 8/22/11 2:05 PM, Jason T. Greene wrote:
> On 8/22/11 1:53 PM, Brian Stansberry wrote:
>> On 8/22/11 1:47 PM, Jason T. Greene wrote:
>>> On 8/22/11 1:42 PM, Jason T. Greene wrote:
>>>> On 8/22/11 1:26 PM, Brian Stansberry wrote:
>>>>> On 8/2/11 4:22 PM, Jason T. Greene wrote:
>>>>>> Currently we have way too many read only attributes in our management
>>>>>> model. This makes life harder for the management tools and/or makes
>>>>>> the
>>>>>> resulting UI a bit unwieldy.
>>>>>>
>>>>>> In some cases this is just because we haven't polished subsystem
>>>>>> management (PLEASE all subsystem authors look at this!). In other
>>>>>> cases
>>>>>> though it's intentional because it is either difficult or currently
>>>>>> impossible to make hot changes to a particular attribute. This
>>>>>> should be
>>>>>> exceedingly rare. Users love hot updates, and it's a competitive
>>>>>> advantage. So first we should strive to make it hot in the first
>>>>>> place.
>>>>>>
>>>>>> However in the case that the change is not hot, and a server
>>>>>> reboot is
>>>>>> too aggressive, we should still supporting making the changes by
>>>>>> bouncing all services involved. After some debate on IRC we finally
>>>>>> arrived at what is IMO a really good option for this problem:
>>>>>>
>>>>>> write-attribute (impact-services=true|false)
>>>>>>
>>>>>> The operation handler on such a non-hot attribute would then
>>>>>> return an
>>>>>> error
>>>>>
>>>>> A handler should not return an error if a change cannot be applied to
>>>>> the runtime. An OperationStepHandler should use the relevant API on
>>>>> the
>>>>> OperationContext. E.g. for a handler registered for Stage.RUNTIME:
>>>>>
>>>>> @Override
>>>>> public void execute(OperationContext context, ModelNode operation) {
>>>>>
>>>>> context.runtimeUpdateSkipped();
>>>>> context.reloadRequired(); // or context.restartRequired();
>>>>> if (context.completeStep() ==
>>>>> OperationContext.ResultAction.ROLLBACK)
>>>>> {
>>>>> context.revertReloadRequired();
>>>>> // or context.revertRestartRequired();
>>>>> }
>>>>> }
>>>>
>>>> Well in my above comment I was referring to a problem where a full
>>>> server reload and/or restart is too aggressive (i.e. why bounce the web
>>>> server when all you want to do is bounce a datasource). So the issue is
>>>> that these attributes don't fit that problem.
>>>
>>> To further clarify the reason to return the error is that the change to
>>> the configuration model is NOT APPLIED. If you touch the config without
>>> the runtime, then you are out of sync and the only reasonable action to
>>> take from that point on is to restart the server. In this case we are
>>> saying that changing an attribute could have hostile consequences
>>> (bouncing a connection pool therby affecting inbound transactions), and
>>> so we need some sort of confirmation by the admin that they really
>>> intend to do this.
>>>
>>
>> I see. Seems this eliminates the ability to make a bunch of config
>> changes without affecting the runtime and then bouncing the server to
>> have them take effect.
>
> Yeah that's a good point. I didn't think of that.
>
>>
>> It sounds like you are looking for 3 possible values for this attribute
>>
>> true -- bounce the affected services
>> false -- apply to config, put server in restart-required state
>> undefined??? -- user made a mistake, return an error
>
> Hmmm, yes I guess you could say there are three states:
>
> ALLOW_SERVICE_RESTART (affects in-flight transactions)
> DELAY_RESTART_UNTIL_NEXT_BOOT
> REJECT_SERVICE_RESTART
>
> On the other hand REJECT_SERVICE_RESTART is really a form of safety
> checking / "babying" that could be pushed to the client, as long as we
> had some kind of metadata to tell them the change affects in-flight
> transactions.
>

It could, although true "babying" is only possible for a console or the 
interactive CLI. A custom client or someone scripting CLI calls couldn't 
be babied.

OT: this discussion implies that a simple "restart-required" -> 
true|false is insufficient metadata for an operation or writable 
attribute. Some ops simply cannot just bounce a service, they must have 
a restart (e.g. those that add/remove DUPs). Some can bounce the 
service. And some don't need to.

-- 
Brian Stansberry
Principal Software Engineer
JBoss by Red Hat