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

Add maximum to number of cookies to store for domain to BdbCookieStore #133

Merged
merged 7 commits into from Nov 11, 2015

Conversation

Projects
None yet
2 participants
@vonrosen
Contributor

vonrosen commented Oct 19, 2015

No description provided.

@nlevitt

This comment has been minimized.

Show comment
Hide comment
@nlevitt

nlevitt Oct 29, 2015

Member

Thanks Hunter. A few thoughts...

  • Rather than log a warning (at level FINEST) and return if there are too many cookies for the domain, maybe addCookie() should throw org.apache.http.cookie.CookieRestrictionViolationException, a subclass of MalformedCookieException, which is caught and logged within the httpclient library.
  • It would be preferable for the per-domain limit to be implemented in org.archive.modules.fetcher.AbstractCookieStore so that SimpleCookieStore and any 3rd party implementations would inherit it.
  • I agree with your comment on the jira that it's too bad you had to add the synchronization to make testConcurrentLoad pass. I don't think we need the synchronization in fact. Because the 50 cookie limit doesn't have to be completely strict. It's just a cutoff to avoid problems, and if a domain has 51 cookies, or even 60, it's unlikely to be a problem. So my suggestion is to remove the synchronization, and change CookieStoreTest:350 to something like assertTrue(domainCounts.get(domain) <= BdbCookieStore.MAX_COOKIES_FOR_DOMAIN + 5);, i.e. check that it's not way over the limit.
  • testConcurrentLoad() doesn't really test what it used to test. I hate to lose these checks entirely: assertTrue(bdbCookieList.size() > 3000); and assertCookieListsEquivalent(bdbCookieList, basicCookieStore().getCookies()). So I would propose having two different concurrent load tests. One that sets cookies on only 10 different domains and checks the domain limits, as your version does now. And another that sets cookies on many different domains, enough domains that no domain hits the 50 cookie limit, but few enough so that there multiple cookies per domain in many cases. Then you can reinstate the checks that were removed. (It might be better to restructure this test to run a certain number of iterations, rather than for a set time.) We can work on it together. I don't think it should be too much work. :)
Member

nlevitt commented Oct 29, 2015

Thanks Hunter. A few thoughts...

  • Rather than log a warning (at level FINEST) and return if there are too many cookies for the domain, maybe addCookie() should throw org.apache.http.cookie.CookieRestrictionViolationException, a subclass of MalformedCookieException, which is caught and logged within the httpclient library.
  • It would be preferable for the per-domain limit to be implemented in org.archive.modules.fetcher.AbstractCookieStore so that SimpleCookieStore and any 3rd party implementations would inherit it.
  • I agree with your comment on the jira that it's too bad you had to add the synchronization to make testConcurrentLoad pass. I don't think we need the synchronization in fact. Because the 50 cookie limit doesn't have to be completely strict. It's just a cutoff to avoid problems, and if a domain has 51 cookies, or even 60, it's unlikely to be a problem. So my suggestion is to remove the synchronization, and change CookieStoreTest:350 to something like assertTrue(domainCounts.get(domain) <= BdbCookieStore.MAX_COOKIES_FOR_DOMAIN + 5);, i.e. check that it's not way over the limit.
  • testConcurrentLoad() doesn't really test what it used to test. I hate to lose these checks entirely: assertTrue(bdbCookieList.size() > 3000); and assertCookieListsEquivalent(bdbCookieList, basicCookieStore().getCookies()). So I would propose having two different concurrent load tests. One that sets cookies on only 10 different domains and checks the domain limits, as your version does now. And another that sets cookies on many different domains, enough domains that no domain hits the 50 cookie limit, but few enough so that there multiple cookies per domain in many cases. Then you can reinstate the checks that were removed. (It might be better to restructure this test to run a certain number of iterations, rather than for a set time.) We can work on it together. I don't think it should be too much work. :)

nlevitt added a commit that referenced this pull request Nov 11, 2015

Merge pull request #133 from vonrosen/ARI-4565
Add maximum to number of cookies to store for domain to BdbCookieStore

@nlevitt nlevitt merged commit 89cd085 into internetarchive:master Nov 11, 2015

@nlevitt

This comment has been minimized.

Show comment
Hide comment
@nlevitt

nlevitt Nov 11, 2015

Member

testConcurrentLoadNoDomainCookieLimitBreach doesn't do "few enough so that there multiple cookies per domain in many cases", but good enough I think :)

Member

nlevitt commented Nov 11, 2015

testConcurrentLoadNoDomainCookieLimitBreach doesn't do "few enough so that there multiple cookies per domain in many cases", but good enough I think :)

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