New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better (randomized) caching to prevent pathological hash collisions #40

Merged
merged 2 commits into from Jan 3, 2018

Conversation

Projects
None yet
2 participants
@pboothe
Contributor

pboothe commented Jan 2, 2018

This change is Reviewable

@pboothe pboothe requested a review from stephen-soltesz Jan 2, 2018

@gfr10598 gfr10598 changed the title from Better caching to prevent systemic hash collisions to Better caching to prevent systematic hash collisions Jan 2, 2018

@pboothe pboothe changed the title from Better caching to prevent systematic hash collisions to Better (randomized) caching to prevent pathological hash collisions Jan 3, 2018

@stephen-soltesz

This comment has been minimized.

Contributor

stephen-soltesz commented Jan 3, 2018

:lgtm: - Very nice.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


paris_rollins.py, line 177 at r1 (raw file):

      now = time.time()
      expiration = now + self._new_wait_time()
      self.cache[address] = expiration

There does not seem to be a usage of the expiration value of the cache. Is that correct? Does this matter? My thought in cases like this is to use the minimally sufficient value, like simply setting the value to True. This prevents new readers or future porters from mistakenly believing there is significance that isn't there.


paris_rollins.py, line 179 at r1 (raw file):

      self.cache[address] = expiration
      heapq.heappush(self.heap, (expiration, address))
      

nit: some extra white space


paris_rollins.py, line 181 at r1 (raw file):

      
  # Returns true if an address was seen too recently.
  def cached(self, address, now=None):

I don't see when now= is ever provided to cached. Have I missed it? Is there a new use planned? If it's not used and not needed please leave it out.


paris_rollins_test.py, line 65 at r1 (raw file):

    ip = self.TEST_DEST_IP
    cache_timeout = 2
    cache = paris_rollins.RecentIPAddressCache(cache_timeout, cache_timeout, cache_timeout)

Please add a comment to the effect that every iteration and add() below exercises the cache expiration and population logic.


Comments from Reviewable

@pboothe

This comment has been minimized.

Contributor

pboothe commented Jan 3, 2018

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.


paris_rollins.py, line 177 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

There does not seem to be a usage of the expiration value of the cache. Is that correct? Does this matter? My thought in cases like this is to use the minimally sufficient value, like simply setting the value to True. This prevents new readers or future porters from mistakenly believing there is significance that isn't there.

Done.


paris_rollins.py, line 179 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

nit: some extra white space

Done.


paris_rollins.py, line 181 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I don't see when now= is ever provided to cached. Have I missed it? Is there a new use planned? If it's not used and not needed please leave it out.

Was previously part of a testing strategy that didn't pan out. Good catch! Removed.


paris_rollins_test.py, line 65 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please add a comment to the effect that every iteration and add() below exercises the cache expiration and population logic.

Done.


Comments from Reviewable

@pboothe pboothe merged commit 83cafd5 into master Jan 3, 2018

3 of 4 checks passed

code-review/reviewable 2 files, 4 discussions left (stephen-soltesz)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 53.515%
Details

@pboothe pboothe deleted the randomized_caching branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment