The changes look thorough, I have a few comments:
- I'm not sure about the Async change on the server service, maybe mazz
can comment. But even if you lose the async calling I think you don't
want to lose the guaranteed delivery. So you may want to consider (I
think we allow this):
@Asynchronous(value = false, guaranteedDelivery = true)
- The check for isCreatable and then the subsequent name update code, I
think this can go above the entityManager.persist(resource);. Very
minor given everything else we do when merging a resource, but it could
save an update if we change the name.
- minor, just wondering why in findMatchingCreateResourceHistory() you
create a query with order by, and not just a usual named query, avoiding
all the pageControl/Ordering field stuff. There should only be at most
one result as resource keys are unique among a parent.
-Jay
On 10/17/2013 6:50 AM, Thomas Segismont wrote:
Hi everyone,
Following your feedback and discussions, I just pushed another
implementation to fix this bug.
https://git.fedorahosted.org/cgit/rhq/rhq.git/commit/?h=bug/1011095&i...
Here is the new logic:
* When an inventory report is merged, we will search for a
CreateResourceHistory entity in the database
* If found, the resource name will be updated with the name hold in
the entity (user supplied name)
* In order to restrain the number of database requests, we will first
check if the newly discovered resource is creatable
If the fix is ok for everyone I will also remove the warning message
in Step 1 of the Create Resource Wizard:
"""
Not all management plug-ins or their managed resources allow the agent
to set the name for a new resource. This value will only be used by
agent plug-ins that support the capability. For plug-ins that do not
support the capability, the resource may receive a generic or
different name when it is discovered.
"""
Important note:
To be able to link the CreateResourceHistory entity and the newly
discovered resource in DiscoveryBossBean, the CreateResourceHistory
entity must hold the new resource key. This means that the PC call to
ResourceFactoryServerService#completeCreateResource must happen
*BEFORE* the inventory report is merged. I found no solution to
guarantee that but to remove the "@Asynchronous(guaranteedDelivery =
true)" annotation from that method.
If we decide to keep the @Asynchronous annotation, the fix should work
in most situations (there should be enough time between the call to
#completeCreateResource and the inventory report).
My opinion is that making it synchronous will not harm.
Regards,
Thomas
Le 24/09/2013 16:33, Jay Shaughnessy a écrit :
>
> I think I agree with Larry. The proposed approach seems very
> band-aid-y. Perhaps we can schedule a call to discuss and see if we can
> brainstorm an alternative.
>
> On 9/24/2013 9:48 AM, Larry O'Leary wrote:
>> On Tue, 2013-09-24 at 12:27 +0200, Thomas Segismont wrote:
>>> Le 23/09/2013 18:01, Larry O'Leary a écrit :
>>>> Why can't the created resource be added to inventory if the create
>>>> request is successful and then allow discovery to merge the
>>>> discovered
>>>> resource and the user created resource? This will prevent the need
>>>> for
>>>> the cache.
>>> Resources are no longer added to the inventory at the end of the
>>> create
>>> resource process since commit 93a54da (9/10/08 - "Don't initially
>>> create
>>> the resource, as the merge from autodiscovery will not update changed
>>> pluginConfigs. JBNADM-3451")
>>>
>> It is this part that I think should be changed. It is understandable
>> that we don't want discovery to update pluginConfig by default.
>> However,
>> updating should be supported for any missing configuration. In other
>> words, any configuration that was not provided by the user in the
>> creation wizard.
>>
>> I don't think one should continue to put bandages on things. Instead, a
>> proper fix should be implemented or a plan to fix such issues in a more
>> meaningful way. This is a good example of such. Is there a reason we
>> don't feel that the user should be able to provide configuration and
>> have it retained?
>
> _______________________________________________
> rhq-devel mailing list
> rhq-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/rhq-devel
_______________________________________________
rhq-devel mailing list
rhq-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/rhq-devel