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