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

TS-4724: Modified ts_lua plugin to provide ts.server_request.remove_h… #859

Closed
wants to merge 1 commit into from
Closed

TS-4724: Modified ts_lua plugin to provide ts.server_request.remove_h… #859

wants to merge 1 commit into from

Conversation

brkishore
Copy link

TS_LUA and InkAPI.cc are modified to create a new ts_lua API to remove host name from URL in GET request to next-tier.

@zwoop zwoop added the Lua label Aug 12, 2016
@zwoop zwoop added this to the 7.0.0 milestone Aug 12, 2016
@brkishore
Copy link
Author

Kit: Could you please back port the commit on to 6.2.x branch if you are satisfied with the changes?

@shukitchan
Copy link
Contributor

For new API in InkAPI.cc, please email to dev@ for an API review. Thanks.
After that I will continue to review the lua related portion.

sdk_assert(sdk_sanity_check_mbuffer(bufp) == TS_SUCCESS);
sdk_assert(sdk_sanity_check_url_handle(obj) == TS_SUCCESS);
URL u;
Debug("cache_url", "[TSUrlRemoveHostName] Removing host name from url");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is "cache_url" the right debug tag? It doesn't seem very obvious to me.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you. Should I use "http_trans" debug tag? Please suggest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "http" should be fine.

@zwoop zwoop closed this Aug 15, 2016
@zwoop zwoop reopened this Aug 15, 2016
@brkishore
Copy link
Author

Kit: I don't see any responses to the email I have sent to the dev@ email. What can I do to get the attention of required experts to comment?

@shukitchan
Copy link
Contributor

I think @jpeach and me replied to your email. Did you not get the response??

Check out here - https://www.mail-archive.com/dev@trafficserver.apache.org/

@shukitchan
Copy link
Contributor

oh and you need to subscribe to the dev@ mailing list to get those emails.

@shukitchan
Copy link
Contributor

In general, this patch is trying to expose the "nuke_proxy_stuff" functionality as an API. And add lua plugin API for that as well.

  1. Can we achieve the same with TSUrl*Set() APIs ?

  2. If yes, then perhaps we don't need a new API.

  3. If yes, then we can simply expose those TSUrl*Set() APIs through ts-lua plugin APIs.

@brkishore
Copy link
Author

Kit: Yes, we can achieve the same functionality using "TSUrlSchemeSet" and "TSUrlHostSet". I will expose these two APIs through ts-lua plugin APIs for "server_request". Should I expose TSUrl*Get() APIs too in ts-lua?

Could you please tell me what can I do to the existing pull request? Should I cancel the existing and raise new one? Please help me understand the right procedure.

@shukitchan
Copy link
Contributor

I think it makes sense. Just make it look similar to "client_request".

I think we can close this one. And please change the title on the jira and open a new pull request with the changes on just ts_lua plugin's "server_request" APIs.

Thanks a lot.

@brkishore brkishore closed this Aug 23, 2016
@brkishore brkishore reopened this Aug 23, 2016
@brkishore
Copy link
Author

Closing this PR as suggested by Kit. New PR #906 is submitted.

@brkishore brkishore closed this Aug 23, 2016
@brkishore brkishore deleted the TS-4724-LuaAPI branch August 23, 2016 19:26
@zwoop zwoop modified the milestone: 7.0.0 May 4, 2017
bneradt added a commit to bneradt/trafficserver that referenced this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants