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

[vonrosen]

No description provided.

[nlevitt]

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]

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