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&id=e950a9b93b952e19e222852f3977b2aa29ef9df3

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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/rhq-devel

_______________________________________________
rhq-devel mailing list
rhq-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/rhq-devel