Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#661 closed defect (fixed)

The rrset entry in cache should be touched/removed properly.

Reported by: zhanglikun Owned by: zhanglikun
Priority: medium Milestone: R-Team-Sprint-20110316
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 6.0 Add Hours to Ticket: 0
Total Hours: 6.0 Internal?: no

Description

Currently, rrset entry in resolver cache isn't touched/removed properly, which may use up the memory, once it expires or lru list is full, the entry at the head of lru list should be removed (also the touch/remove operation for nsas entry and message entry should be revisited properly.), assign this ticket to me, it's a little urgent.

Subtickets

Change History (11)

comment:1 Changed 50 years ago by zhanglikun

  • Add Hours to Ticket changed from 0.0 to 6.0
  • Total Hours changed from 0.0 to 6.0

comment:1 Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to ocean
  • Status changed from new to reviewing

comment:2 Changed 9 years ago by ocean

src/lib/cache/rrset_cache.cc
src/lib/cache/message_entry.cc
When the RRsetEntry is expired and replace with new entry, it should be also removed from the LRU list, otherwise, it will occupy one space of LRU list until it is pushed to the front.

comment:3 Changed 9 years ago by ocean

  • Owner changed from ocean to zhanglikun

comment:4 follow-ups: Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to ocean

well, the following change has been committed to trac661, please have a look.
When looking up the message or rrset in the cache, expired entries will be removed from the cache and lru list automatically. Now the expired entries will be removed until it is looked up, or dropped when adding a new entry to the cache and the cache lru list is full, or overwritten by a new entry.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by ocean

Replying to zhanglikun:

well, the following change has been committed to trac661, please have a look.
When looking up the message or rrset in the cache, expired entries will be removed from the cache and lru list automatically. Now the expired entries will be removed until it is looked up, or dropped when adding a new entry to the cache and the cache lru list is full, or overwritten by a new entry.

Suggest to refactor the function of RRsetCache::update() to:

 if(entry_ptr && entry_ptr->getTrustLevel() > level) {
    return (entry_ptr);
 }
 
 // Replace the old entry with higher trust-level entry
 rrset_lru_.remove(entry_ptr);
 entry_ptr.reset(new RRsetEntry(rrset, level));
 ...

The logic will be more clear and the code will be more readable

Last edited 9 years ago by ocean (previous) (diff)

comment:6 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by ocean

lib/cache/test/testdata/message_fromewire9
The comments need to update

lib/cache/test/rrset_cache_unittest.cc
In function TEST_F(RRsetCacheTest, lookup)

updateRRsetCache(cache_, name_test, 0);

suggest changes to

updateRRsetCache(cache_, name_test, RRSET_TRUST_DEFAULT);

comment:7 Changed 9 years ago by ocean

  • Owner changed from ocean to zhanglikun

comment:8 in reply to: ↑ 5 Changed 9 years ago by zhanglikun

Replying to ocean:

Replying to zhanglikun:

well, the following change has been committed to trac661, please have a look.
When looking up the message or rrset in the cache, expired entries will be removed from the cache and lru list automatically. Now the expired entries will be removed until it is looked up, or dropped when adding a new entry to the cache and the cache lru list is full, or overwritten by a new entry.

Suggest to refactor the function of RRsetCache::update() to:

 if(entry_ptr && entry_ptr->getTrustLevel() > level) {
    return (entry_ptr);
 }
 
 // Replace the old entry with higher trust-level entry
 rrset_lru_.remove(entry_ptr);
 entry_ptr.reset(new RRsetEntry(rrset, level));
 ...

The logic will be more clear and the code will be more readable

We have to check if the entry_ptr is empty before removing it from lru list.

comment:9 in reply to: ↑ 6 Changed 9 years ago by zhanglikun

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to ocean:

lib/cache/test/testdata/message_fromewire9
The comments need to update

lib/cache/test/rrset_cache_unittest.cc
In function TEST_F(RRsetCacheTest, lookup)

updateRRsetCache(cache_, name_test, 0);

suggest changes to

updateRRsetCache(cache_, name_test, RRSET_TRUST_DEFAULT);

The 0 is TTL, not rrset trust level, anyway, I have added the comment to it in the code.
The ticket has been merged to master(git 9efbe64fe3ff22bb5fba46de409ae058f199c8a7), so close this one.

comment:10 Changed 9 years ago by zhanglikun

  • Add Hours to Ticket changed from 0 to 6
  • Estimated Difficulty changed from 0.0 to 6
Note: See TracTickets for help on using tickets.