You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by masaori335 <gi...@git.apache.org> on 2016/07/25 06:26:17 UTC
[GitHub] trafficserver pull request #825: TS-4694: Some refactoring after SPDY is rem...
GitHub user masaori335 opened a pull request:
https://github.com/apache/trafficserver/pull/825
TS-4694: Some refactoring after SPDY is removed
- Remove PluginIdentity class from base classes of Http2ClientSession
- Add get_protocol_string() to ProxyClientSession to identify if the session is HTTP/2 or HTTP/1.x
- Add "client_protocol" to HttpSM to track client protocol versions
- Drop HTTP/0.9 support from cqpv (HTTP/0.9 is already dropped by TS-3327)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/masaori335/trafficserver ts-4694
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/825.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 #825
----
commit 4a9c564ff7727cab7929a29e38f5ff5ae0c62c62
Author: Masaori Koshiba <ma...@apache.org>
Date: 2016-07-22T06:17:50Z
TS-4694: Some refactoring after SPDY is removed
- Remove PluginIdentity class from base classes of Http2ClientSession
- Add get_protocol_string() to ProxyClientSession to identify if the session is HTTP/2 or HTTP/1.x
- Add "client_protocol" to HttpSM to track client protocol versions
- Drop HTTP/0.9 support from cqpv (HTTP/0.9 is already dropped by TS-3327)
----
---
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 issue #825: TS-4694: Some refactoring after SPDY is removed
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/825
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/377/ for details.
---
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 #825: TS-4694: Some refactoring after SPDY is rem...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/825#discussion_r72022413
--- Diff: proxy/ProxyClientTransaction.h ---
@@ -211,7 +211,11 @@ class ProxyClientTransaction : public VConnection
virtual bool allow_half_open() const = 0;
- virtual const char *get_protocol_string() const = 0;
+ virtual const char *
--- End diff --
Would it make sense to have these getter's take an (optional) argument to get the length as well? I noticed that later, in e.g. logging, we have to do strlen() on these static strings every time we use, which seems a bit wasteful, no? Maybe something like
get_protocol_string(int *len = NULL);
?
---
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 issue #825: TS-4694: Some refactoring after SPDY is removed
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/825
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/480/ for details.
---
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 issue #825: TS-4694: Some refactoring after SPDY is removed
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/825
:+1:
---
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 #825: TS-4694: Some refactoring after SPDY is rem...
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/825#discussion_r72179084
--- Diff: proxy/ProxyClientTransaction.h ---
@@ -211,7 +211,11 @@ class ProxyClientTransaction : public VConnection
virtual bool allow_half_open() const = 0;
- virtual const char *get_protocol_string() const = 0;
+ virtual const char *
--- End diff --
Yeah, it's better to reduce strlen calls. I start thinking about returning int or enum from here (something like `TS_SSN_PROTOCOL_INDEX_HTTP_x_x`) and convert it to string in logging phase.
Do we really need to return string at here?
---
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 #825: TS-4694: Some refactoring after SPDY is rem...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/825#discussion_r72122764
--- Diff: proxy/ProxyClientTransaction.h ---
@@ -211,7 +211,11 @@ class ProxyClientTransaction : public VConnection
virtual bool allow_half_open() const = 0;
- virtual const char *get_protocol_string() const = 0;
+ virtual const char *
--- End diff --
I don't think adding a length argument makes much difference one way or the other. I guess it would reduce the logging call site to once call. But we are storing the protocol as a NULL terminated string, so we'd be doing the strlen in any case.
---
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 issue #825: TS-4694: Some refactoring after SPDY is removed
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/825
Looks good to me as well.
---
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 #825: TS-4694: Some refactoring after SPDY is rem...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/825#discussion_r72140971
--- Diff: proxy/ProxyClientTransaction.h ---
@@ -211,7 +211,11 @@ class ProxyClientTransaction : public VConnection
virtual bool allow_half_open() const = 0;
- virtual const char *get_protocol_string() const = 0;
+ virtual const char *
--- End diff --
I'm ok without it, but the way logging works, you will do strlen() on that string twice every time you log it (once to calculate the buffer space needed, and once to write the string).
---
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 issue #825: TS-4694: Some refactoring after SPDY is removed
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the issue:
https://github.com/apache/trafficserver/pull/825
Looks good
---
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 #825: TS-4694: Some refactoring after SPDY is rem...
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/825#discussion_r72222506
--- Diff: proxy/ProxyClientTransaction.h ---
@@ -211,7 +211,11 @@ class ProxyClientTransaction : public VConnection
virtual bool allow_half_open() const = 0;
- virtual const char *get_protocol_string() const = 0;
+ virtual const char *
--- End diff --
Currently plugins can not set client_protocol of HttpSM ( which is going to be added by this patch ), but it could be set in the future. And it also has consistency for other stats in HttpSM (e.g. client_sec_protocol, client_cipher_suite). So, let's leave it string.
I'll file a new tickets for optimization.
---
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 #825: TS-4694: Some refactoring after SPDY is rem...
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 closed the pull request at:
https://github.com/apache/trafficserver/pull/825
---
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 #825: TS-4694: Some refactoring after SPDY is rem...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/825#discussion_r72200488
--- Diff: proxy/ProxyClientTransaction.h ---
@@ -211,7 +211,11 @@ class ProxyClientTransaction : public VConnection
virtual bool allow_half_open() const = 0;
- virtual const char *get_protocol_string() const = 0;
+ virtual const char *
--- End diff --
Can plugins set the string? If so, an enum would not suffice, right?
---
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.
---