You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by jacksontj <gi...@git.apache.org> on 2016/03/15 17:16:46 UTC
[GitHub] trafficserver pull request: Fix for TS-4276
GitHub user jacksontj opened a pull request:
https://github.com/apache/trafficserver/pull/526
Fix for TS-4276
In the event that `lookup_done` returns a NULL, we'll reschedule the lookup in the future instead of dumping core.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jacksontj/trafficserver master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/526.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #526
----
commit 7add326f5020e897bf1ff15b7d9eca14a1adf9dc
Author: Thomas Jackson <ja...@gmail.com>
Date: 2016-03-15T16:15:53Z
Fix for TS-4276
In the event that `lookup_done` returns a NULL, we'll reschedule the lookup in the future instead of dumping core.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/526#issuecomment-196921878
As I'm thinking through this some more, I'm not certain this is a great solution either. The particular case that I reproduced was that hostdb was full when returning rr() records. It seems that the `insert` into hostdb can't fail (as it will eject things if necessary), so really the core issue here is that we are trying to store some additional information (hostnames, etc.) and we cannot store them because we are out of space. The current patch I have here is just returning the HostDBInfo struct, but it didn't put anything in where the hostnames should be. In the more simple cases (where we are storing the hostname that we looked up) it is probably preferable to return without a hostname at all, but in the reverse DNS case that is the primary intent of the lookup.
Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/526#issuecomment-197047822
Not sure I understand. I'm saying, evict before it gets full, so normal eviction happens when there's 10% of less available, before inserting new stuff. That gives a 10% head room for this "overallocation" where you can't (maybe?) easily evict more stuff to make room for the necessary changes ?
But if 1) is easily done, such that you can just evict more when needed, I'm +1 on that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/526#issuecomment-196907474
This is a different patch to the same issue. Instead of scheduling it for a retry, we can just return the non-mmaped HostDBInfo we have, so everything else can continue to function in a non-persistant way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/526#issuecomment-196904843
I'm not sure this is the correct fix, in this case where we have no space we probably should just get lookup_done to return a non-persistant version of the record instead of just retrying. In this retry case, lookups will continue to fail until a vacancy shows up in hostdb.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj closed the pull request at:
https://github.com/apache/trafficserver/pull/526
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/526#issuecomment-199878278
I don't want to merge this PR in, its a definite hack. I'm working on a better fix, it'll just take some time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/526#issuecomment-197011767
I don't think we can ever gaurantee that there will always be space available, and since we already evict when we need space, we should just keep doing that IMO.
I think we should basically do 1, but specifically as an atomic insert. Right now we are inserting N (usually 2-3) items into MultiCache. Instead we could allocate all of the space up front, and insert just a single item-- this way we are atomic with our objects in cache.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/526#issuecomment-196975636
Maybe it needs to evict things more aggressively, such that it always has ~10% head room for these sort of events ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/526#issuecomment-199871069
@SolidWallOfCode to review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Fix for TS-4276
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/526#issuecomment-197055859
From what I'm seeing 1) should be completely doable-- I'll take a crack at it :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---