Github user marcaurele commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1532#discussion_r62453258
--- Diff: framework/db/src/com/cloud/utils/db/GenericDaoBase.java ---
@@ -969,7 +969,12 @@ public T findByUuidIncludingRemoved(final String uuid)
{
@Override
@DB()
public T findByIdIncludingRemoved(ID id) {
- return findById(id, true, null);
+ if (_cache != null) {
+ final Element element = _cache.get(id);
+ return element == null ? findById(id, true, null) :
(T)element.getObjectValue();
+ } else {
--- End diff --
The test for the existence of the cache is made on other lines in this
class, so I kept it. My change is to use the cached value if there is one, as
it wasn't the case before.
The function looks pretty much the same as the `findById` (line 944).
@DaanHoogland in your last code sample, on the first line you go first to
the DB to retrieve the object. Then if the cache isn't null, you fetch the
value from the cache and discard the one you just got. IMO it's a downgrade in
terms of performance as you never use directly the value in cache but always do
a query against the DB.
@jburwell I could change the code as you're suggesting, but then the
exception should be raised at other places too to keep the code consistent when
the cache isn't configured, which wasn't my initial goal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
