DAO: Hit the cache for entity flagged as removed too #1532

marcaurele · 2016-05-04T12:43:33Z

I came along this part of the code and I don't see any reason why the cache should not be used when fetching with the "removed" ones. It will help decrease the number of DB queries.

It can be merged in many CS versions

in cache afterwards.

jburwell · 2016-05-06T08:47:20Z

framework/db/src/com/cloud/utils/db/GenericDaoBase.java

+ if (_cache != null) {
+ final Element element = _cache.get(id);
+ return element == null ? findById(id, true, null) : (T)element.getObjectValue();
+ } else {


This else clause is unnecessary due the return in preceding if clause. Please remove it to simplify the code.

@jburwell if _cache == null a return needs to be there (or a throw of some sort)

@dahn if the _cache is not optional then I agree. It should probably be something like the following:

if (_cache == null) { throw new IllegalStateException("The cache has not been properly initialized for DAO " + this.getClass().getSimpleName()); } final Element element = _cache.get(id); return element == null ? findById(id, true, null) : (T) element.getObjectValue();

Would you agree?

yes, but I think the submitted code suggest it is optional:

T result = findById(id, true, null) if (_cache != null) { final Element element = _cache.get(id); if (element != null) { result = (T)element.getObjectValue(); } } return result;

I have no problem with the proposed code if _cache is optional however.

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.

@marcaurele good point so to address your concern:

T result = null if (_cache != null) { final Element element = _cache.get(id); if (element != null) { result = (T)element.getObjectValue(); } } else { result =findById(id, true, null); } return result;

this also implements the good practice of having only one return in the method. I am not hellbound on these changes however. As the method is so small it would be merely cosmetic.

sateesh-chodapuneedi · 2016-05-09T09:12:58Z

@DaanHoogland I agree that's the best practice though there is a glitch here. Following code returns null if _cache is not null but element is null,

    T result = null
    if (_cache != null) {
        final Element element = _cache.get(id);
        if (element != null) {
            result = (T)element.getObjectValue();
        }
    } else {
        result =findById(id, true, null);
    }
    return result;

sateesh-chodapuneedi · 2016-05-09T09:15:01Z

Probably intention is to,

    T result = null
    if (_cache != null) {
        final Element element = _cache.get(id);
        if (element != null) {
            result = (T)element.getObjectValue();
        } else {
            result = findById(id, true, null);
        }
    }
    return result;

marcaurele · 2016-05-09T09:41:20Z

@sateesh-chodapuneedi not quite, you're missing a case. If the cache is null (not configured) you will always return null, which is not the goal.

marcaurele · 2016-05-09T09:42:58Z

I pushed a change to write the method following the good practive you're mentioning. I also updated the sibling method findById to follow the same practice.

DaanHoogland · 2016-05-09T10:24:41Z

@marcaurele @sateesh-chodapuneedi You are both right probably more then I will ever be so please take with a grain of salt:

    T result = null;
    if (_cache != null) {
        final Element element = _cache.get(id);
        if (element != null) {
            result = (T)element.getObjectValue();
        }
    }
    if(result == null) {
        result = lockRow(id, null);
    }
    return result

same pattern would apply to the findIncludingRemoved variety. I think this is the cleanest though Marc's version should be semantically equal given a good optimising compiler.
@swill I will start integration tests on this next.

DaanHoogland · 2016-05-09T11:14:40Z

framework/db/src/com/cloud/utils/db/GenericDaoBase.java

 }
+ return result


a ';' missing :(

DaanHoogland · 2016-05-09T13:41:13Z

LGTM based on code, CI pending

swill · 2016-05-09T22:02:55Z

Thanks @DaanHoogland. 👍

DaanHoogland · 2016-05-10T13:49:17Z

@swill I am getting to many false positives in my environment to grant it value, can you schedule this for integration testing?

swill · 2016-05-10T14:12:51Z

@DaanHoogland I am also getting more false negatives than I had in the past. I am also pushing my environments a bit harder than I was before, so I think that is part of it. I will add this one to my CI.

swill · 2016-05-11T05:46:37Z

CI RESULTS

Tests Run: 94
  Skipped: 0
   Failed: 2
   Errors: 0
 Duration: 7h 45m 13s

Summary of the problem(s):

FAIL: Create a redundant VPC with 1 Tier, 1 VM, 1 ACL, 1 PF and test Network GC Nics
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs2/cloudstack/test/integration/smoke/test_vpc_redundant.py", line 605, in test_04_rvpc_network_garbage_collector_nics
    self.check_routers_state(status_to_check="BACKUP", expected_count=2)
  File "/data/git/cs2/cloudstack/test/integration/smoke/test_vpc_redundant.py", line 353, in check_routers_state
    self.fail("Expected '%s' routers at state '%s', but found '%s'!" % (expected_count, status_to_check, cnts[vals.index(status_to_check)]))
AssertionError: Expected '2' routers at state 'BACKUP', but found '1'!
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_BG7UFE/results.txt

FAIL: test_04_rvpc_privategw_static_routes (integration.smoke.test_privategw_acl.TestPrivateGwACL)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/git/cs2/cloudstack/test/integration/smoke/test_privategw_acl.py", line 277, in test_04_rvpc_privategw_static_routes
    self.performVPCTests(vpc_off)
  File "/data/git/cs2/cloudstack/test/integration/smoke/test_privategw_acl.py", line 324, in performVPCTests
    self.check_pvt_gw_connectivity(vm1, public_ip_1, vm2.nic[0].ipaddress)
  File "/data/git/cs2/cloudstack/test/integration/smoke/test_privategw_acl.py", line 559, in check_pvt_gw_connectivity
    "Ping to outside world from VM should be successful"
AssertionError: Ping to outside world from VM should be successful
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_BG7UFE/results.txt

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__May_10_2016_19_40_27_1Z9M75:

/tmp/MarvinLogs/test_network_BG7UFE:

/tmp/MarvinLogs/test_privategw_acl_JUTOR4:

/tmp/MarvinLogs/test_vpc_routers_009P6U:

Uploads will be available until 2016-07-11 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-11T05:49:33Z

I reran the tests that failed and they succeeded the second time, so CI is in a good place on this one. I need one more code review on this one. @marcaurele can you please force push this PR to kick of Jenkins again so when we get another code review we can merge right away. Thanks...

marcaurele · 2016-05-11T06:12:43Z

@swill did a force push by changing the last commit description

swill · 2016-05-11T13:17:58Z

Thank you sir. Unfortunately travis failed this time. Would you mind trying it again. Thanks...

DaanHoogland · 2016-05-11T14:14:16Z

@marcaurele instead of force push you can close and reopen after a few seconds. This usually works as well and seves you switching windows browser->terminal->browser ;)

marcaurele · 2016-05-12T11:32:28Z

I don't get what's the relation between my change and the jenkins build failling. Should I trigger another one?

DaanHoogland · 2016-05-12T11:40:28Z

@marcaurele I don't think it is related to your code either. can you open/close your pr to see if it works. If not, maybe we need to empty the jenkins-workspace. The problems are not clear to me, might be jenkins, our job or something messy in our code. Any pointers are welcome.

swill · 2016-05-12T12:32:36Z

Once I get #1539 merged, I will ask you to re-push since we are working to fix the issue that is causing Travis to fail in that PR right now. Thx...

swill · 2016-05-12T16:09:31Z

@marcaurele I have merged fixes to Jenkins and Travis, can you force push again to kick off those runs again. Sorry for the inconvenience, hopefully this is the last time. :P

swill · 2016-05-12T20:42:38Z

Sorry to do this to you again @marcaurele. Can you rebase and force push again. I have disabled the test in master that was blocking this in Jenkins because we have verified that there are problems with that test. Thx...

Also change the sibling method findById(final ID id)

marcaurele · 2016-05-13T10:46:04Z

checks are finally ok...

swill · 2016-05-13T16:30:00Z

I am counting my own code review on this one given the scope of the change and the fact that a review is the only thing holding this up. I will merge this one...

DaanHoogland · 2016-05-13T21:03:01Z

@swill you should always count the opinion of the RM as that of a real memeber of the community ;)

DAO: Hit the cache for entity flagged as removed tooI came along this part of the code and I don't see any reason why the cache should not be used when fetching with the "removed" ones. It will help decrease the number of DB queries. *It can be merged in many CS versions* * pr/1532: DAO: Rewrite change for method findByIdIncludingRemoved(ID id) dao: Hit the cache for entity flagged as removed too since they are put in cache afterwards. Signed-off-by: Will Stevens <[email protected]>

dao: Hit the cache for entity flagged as removed too since they are put

696440a

in cache afterwards.

jburwell reviewed May 6, 2016
View reviewed changes

DaanHoogland reviewed May 9, 2016
View reviewed changes

framework/db/src/com/cloud/utils/db/GenericDaoBase.java

}

return result

Copy link

Contributor

DaanHoogland May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a ';' missing :(

marcaurele force-pushed the fix/db-cache-miss branch from 4682381 to e7ad942 Compare May 9, 2016 12:18

marcaurele force-pushed the fix/db-cache-miss branch from e7ad942 to 56fe0ec Compare May 11, 2016 06:11

marcaurele closed this May 11, 2016

marcaurele reopened this May 11, 2016

marcaurele force-pushed the fix/db-cache-miss branch from 56fe0ec to 648e06c Compare May 12, 2016 11:57

marcaurele force-pushed the fix/db-cache-miss branch from 648e06c to 28d0bd3 Compare May 12, 2016 19:30

marcaurele force-pushed the fix/db-cache-miss branch from 28d0bd3 to 2685eb4 Compare May 13, 2016 06:19

DAO: Rewrite change for method findByIdIncludingRemoved(ID id)

39aa0e4

Also change the sibling method findById(final ID id)

marcaurele force-pushed the fix/db-cache-miss branch from 2685eb4 to 39aa0e4 Compare May 13, 2016 09:01

asfgit merged commit 39aa0e4 into apache:master May 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAO: Hit the cache for entity flagged as removed too #1532

DAO: Hit the cache for entity flagged as removed too #1532

marcaurele commented May 4, 2016

jburwell May 6, 2016

DaanHoogland May 6, 2016

jburwell May 6, 2016

DaanHoogland May 6, 2016

marcaurele May 9, 2016

DaanHoogland May 9, 2016

sateesh-chodapuneedi commented May 9, 2016

sateesh-chodapuneedi commented May 9, 2016 •

edited

Loading

marcaurele commented May 9, 2016

marcaurele commented May 9, 2016

DaanHoogland commented May 9, 2016

DaanHoogland May 9, 2016

DaanHoogland commented May 9, 2016

swill commented May 9, 2016

DaanHoogland commented May 10, 2016

swill commented May 10, 2016

swill commented May 11, 2016

swill commented May 11, 2016

marcaurele commented May 11, 2016

swill commented May 11, 2016

DaanHoogland commented May 11, 2016

marcaurele commented May 12, 2016

DaanHoogland commented May 12, 2016

swill commented May 12, 2016

swill commented May 12, 2016

swill commented May 12, 2016

marcaurele commented May 13, 2016

swill commented May 13, 2016

DaanHoogland commented May 13, 2016

DAO: Hit the cache for entity flagged as removed too #1532

DAO: Hit the cache for entity flagged as removed too #1532

Conversation

marcaurele commented May 4, 2016

jburwell May 6, 2016

Choose a reason for hiding this comment

DaanHoogland May 6, 2016

Choose a reason for hiding this comment

jburwell May 6, 2016

Choose a reason for hiding this comment

DaanHoogland May 6, 2016

Choose a reason for hiding this comment

marcaurele May 9, 2016

Choose a reason for hiding this comment

DaanHoogland May 9, 2016

Choose a reason for hiding this comment

sateesh-chodapuneedi commented May 9, 2016

sateesh-chodapuneedi commented May 9, 2016 • edited Loading

marcaurele commented May 9, 2016

marcaurele commented May 9, 2016

DaanHoogland commented May 9, 2016

DaanHoogland May 9, 2016

Choose a reason for hiding this comment

DaanHoogland commented May 9, 2016

swill commented May 9, 2016

DaanHoogland commented May 10, 2016

swill commented May 10, 2016

swill commented May 11, 2016

CI RESULTS

swill commented May 11, 2016

marcaurele commented May 11, 2016

swill commented May 11, 2016

DaanHoogland commented May 11, 2016

marcaurele commented May 12, 2016

DaanHoogland commented May 12, 2016

swill commented May 12, 2016

swill commented May 12, 2016

swill commented May 12, 2016

marcaurele commented May 13, 2016

swill commented May 13, 2016

DaanHoogland commented May 13, 2016

sateesh-chodapuneedi commented May 9, 2016 •

edited

Loading