Skip to content
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

Resolver: using Fully qualified names. #1235

Merged
merged 1 commit into from
Oct 29, 2020
Merged

Conversation

eloycoto
Copy link
Contributor

When adding a dot to the end of the domain, the search string from
resolv.conf should not be added, because the domain is absolute.

Adding this, will save a few DNS request to the DNS server because no
cluster.local, etc.. will be added to the Qname.

More info in relation to this:
https://superuser.com/a/1468139

Fix THREESCALE-4752

Signed-off-by: Eloy Coto [email protected]

@eloycoto eloycoto requested a review from a team as a code owner October 27, 2020 21:46
return nil, err
end

if string.sub(qname, -1) == "." then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not optimal. Would be better to cache the string.sub to a local variable. The same for qname:sub.

https://www.youtube.com/watch?v=FfhEdF40nhQ

return nil, err
end

local qname_last_char = string.sub(qname, -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not optimal. Would be better to cache the string.sub to a local variable. The same for goes for qname:sub. Not the output of the call, but the function being called local sub = string.sub.

https://www.youtube.com/watch?v=FfhEdF40nhQ

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained in the video, using string.sub needs to lookup string, which is in _G and going that far up in stack breaks JIT.

This might not be on a hot path, but better make it everywhere, so you don't have to think about it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sleepy morning :-( Fixed it now 😅

@eloycoto eloycoto force-pushed the 4752 branch 3 times, most recently from dc4a00f to 68454f7 Compare October 28, 2020 16:48
When adding a dot to the end of the domain, the search string from
resolv.conf should not be added, because the domain is absolute.

Adding this, will save a few DNS request to the DNS server because no
cluster.local, etc.. will be added to the Qname.

More info in relation to this:
https://superuser.com/a/1468139

Fix THREESCALE-4752

Signed-off-by: Eloy Coto <[email protected]>
@eloycoto eloycoto merged commit 87d1e54 into 3scale:master Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants