Skip to content
This repository has been archived by the owner on Jul 11, 2022. It is now read-only.

BZ1032192 - AlertCondition composite queries, query each column #65

Merged
merged 2 commits into from
Aug 19, 2014

Conversation

genman
Copy link
Contributor

@genman genman commented Jun 24, 2014

Hibernate would return an empty entity and consequently lead to N+1 selects.

The code is ugly, unfortunately.

Adding tests for these queries

Hibernate would return an empty entity and consequently lead to N+1 selects.

The code is ugly, unfortunately.

Adding tests for these queries
@metlos
Copy link
Contributor

metlos commented Jun 24, 2014

Seeing some constructor removals and such in domain classes... Could you run this through an API check, pls? https://docs.jboss.org/author/display/RHQ/Tracking+API+Changes

Adding intentional API changes as well
@genman
Copy link
Contributor Author

genman commented Jun 25, 2014

Okay, I added the API change stuff. I'm not sure if the Composite constructors really should be part of the public domain classes.

@metlos
Copy link
Contributor

metlos commented Jun 26, 2014

Quite a bit in the core/domain has no place in there but that's a legacy burden we unfortunately have to deal with.

@jshaughn
Copy link
Contributor

Instead of the individual columns, would it have been possible to keep the entity in the select column and add a "left join fetch ac" ?

@genman
Copy link
Contributor Author

genman commented Jun 27, 2014

Doesn't work. Feel free to try, I couldn't get it to work.

On Friday, June 27, 2014, jshaughn notifications@github.com wrote:

Instead of the individual columns, would it have been possible to keep the
entity in the select column and add a "left join fetch ac" ?


Reply to this email directly or view it on GitHub
https://github.com/rhq-project/rhq/pull/65#issuecomment-47353261.[image:
Web Bug from
https://github.com/notifications/beacon/702278__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxOTQ5OTM4NiwiZGF0YSI6eyJpZCI6MzU0NDM4NDh9fQ==--99ab5388a0ff823b068a73d91fde3920ecce31a6.gif]

This message has been scanned for viruses and
dangerous content by MailScanner http://www.mailscanner.info/, and is
believed to be clean.

@jshaughn
Copy link
Contributor

If you tried that approach I'll take your word for it. Although the code is more verbose, and a little more fragile since changes to AlertCondition fields will probably affect all of the composite constructors, it's consistent and not difficult to understand, so I'll do a quick check to make sure this still looks good against master and if the test goes well, I'll merge it in.

@jshaughn
Copy link
Contributor

Test went well, merging into master... Thanks, Elias!

jshaughn added a commit that referenced this pull request Aug 19, 2014
BZ1032192 - AlertCondition composite queries, query each column
@jshaughn jshaughn merged commit ab6e5c0 into rhq-project:master Aug 19, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants