Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
[approve ci]
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
[approve ci]
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
please check the failure.
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
[approve ci]
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
it looks good to me.
@bryancall @zwoop can either of you guys take a look, too? thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
[approve ci]
---
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
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1557#discussion_r107666043
--- Diff: plugins/gzip/gzip.cc ---
@@ -115,20 +142,32 @@ gzip_data_destroy(GzipData *data)
}
static TSReturnCode
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1516
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
wouldn't it be easier if "enforce-brotli" will override the
"Accept-Encoding" to "br"?
So the origin will either return plain text or brotli encoded d
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
two more points
1) I think we need to maintain the existing behavior as much as possible.
So perhaps the default for the new configuration option "supported-algorithms&quo
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1516
can you squash now? then i can verify and merge.
---
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
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1516#discussion_r106500402
--- Diff: doc/admin-guide/plugins/combo_handler.en.rst ---
@@ -83,3 +83,16 @@ results in these file paths being "reconstr
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1516#discussion_r106484837
--- Diff: doc/admin-guide/plugins/combo_handler.en.rst ---
@@ -83,3 +83,16 @@ results in these file paths being "reconstr
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
@myraid your description seems to be good. i will spend some times tomorrow
to review the code.
meanwhile, pls update this as well
https://github.com/apache/trafficserver
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1516
looks good. thanks.
can you update this as well?
https://github.com/apache/trafficserver/blob/master/doc/admin-guide/plugins/combo_handler.en.rst
@SolidWallOfCode , any
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/issues/1506
The fix is merged
---
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
Github user shukitchan closed the issue at:
https://github.com/apache/trafficserver/issues/1506
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
My feedbacks
1) Perhaps we are overloading the "Accept-Encoding" header in the request.
Let's say we create another header called "X-Normalized-Accept-Encodi
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1552
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1552
it 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
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1557#discussion_r106037813
--- Diff: plugins/gzip/gzip.cc ---
@@ -350,30 +435,89 @@ gzip_transform_finish(GzipData *data)
if (data->downstream_length != (int6
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1520
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1520
#1534 is merged. So closing this one. Thanks @SolidWallOfCode
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1516
feature flag or feature gate should be fine.
i think it is a bug, too.
but unfortunately i think we still need to support that till the next major
release.
---
If your project
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1557
I will take a closer look in the coming days.
In the meanwhile, can you update the documentation for the gzip plugin as
well for this change?
---
If your project is set up
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1516
I think we should keep the existing behavior since this is not an
experimental plugin and we are not in the middle of releasing a major version.
---
If your project is set up
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1551
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1551
thanks for spotting the mistake.
ð
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1534
ð
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1534
ð
Looks good.
Just one question. Should the internal "protocol_contains" functions return
"StringView" as well?
Obviously we should not be chan
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1520#discussion_r104270172
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -741,26 +741,14 @@
HttpTransactHeaders::insert_via_header_in_request(HttpTransact::State *s
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1520#discussion_r104076857
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -741,26 +741,14 @@
HttpTransactHeaders::insert_via_header_in_request(HttpTransact::State *s
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1158
it looks like .luaconf is the winner.
I will be updating this accordingly. Also will create new issue to make
sure metrics.config and logging.config are changed accordingly as well
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1516
the patch compiles fine. And it seems to run through clang-format as well
already. thanks. pls check out the comments above.
---
If your project is set up for it, you can reply
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1516#discussion_r103590528
--- Diff: plugins/esi/combo_handler.cc ---
@@ -187,6 +207,94 @@ InterceptData::~InterceptData()
}
}
+void
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1516
will try to compile it tonight and see if i have more comments.
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1516
The max-age related code is also a bit different from before. Previously we
can have max-age larger than 31536. Now with this change, I think we can no
longer have max-age larger than
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1516
I am not in favor with the "private" related changes because i think it can
catch some existing users by surprise. And we did not mention anything in the
document as wel
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1516#discussion_r103587579
--- Diff: plugins/esi/combo_handler.cc ---
@@ -136,6 +138,24 @@ struct InterceptData {
~InterceptData
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1516#discussion_r103586416
--- Diff: plugins/esi/combo_handler.cc ---
@@ -136,6 +138,24 @@ struct InterceptData {
~InterceptData
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1516
I would like to do the review on this.
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/issues/1506
interesting. That's done in the response "Via" header. But not in the
request "Via" header.
https://github.com/apache/trafficserver/blob/master/proxy/http/Ht
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/776
I personally don't mind doing both. i.e. get this PR done right to merge
and also changing gzip plugin to support brotli. How does that sound?
---
If your project is set up for it, you
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/776
You can see @PSUdaemon comment above. That's pretty much the idea behind
the efforts of rewriting it to merge with gzip plugin.
---
If your project is set up for it, you can reply
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/776
discussed with Bryan.
We will submit a PR that will change the gzip plugin instead of a separate
plugin like this PR.
---
If your project is set up for it, you can reply
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1278
---
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
GitHub user shukitchan opened a pull request:
https://github.com/apache/trafficserver/pull/1278
TS-5101: fix error handling for ts_lua
@jpeach @zwoop
pls help to review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1277
---
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
GitHub user shukitchan opened a pull request:
https://github.com/apache/trafficserver/pull/1277
TS-5041: fix leak in ts_lua plugin
@jpeach , pls help to review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shukitchan
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1261
ð
---
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
Github user shukitchan closed the issue at:
https://github.com/apache/trafficserver/issues/1251
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/issues/1251
oh. also if you set the host/port in read request hook (i.e. before mapping
happens), it may not match the second line at all (the one with the lua
plugin).
It may leverage
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/issues/1251
i mean if you use other remap plugins in your remap rule (e.g. conf_remap)
, do they also not work?
just trying to see if it is a ts_lua specific problem or not.
---
If your
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/issues/1251
is it only ts-lua plugin not working? or all remap plugins are not working?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/issues/1251
If you called TSSkipRemappingSet() after doing those Set functions, then
the remap related functionality will be skipped.
---
If your project is set up for it, you can reply
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1217
---
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
GitHub user shukitchan opened a pull request:
https://github.com/apache/trafficserver/pull/1217
TS-5041: fix resource leak in ts_lua
@zwoop please take a look. Thanks.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shukitchan
GitHub user shukitchan opened a pull request:
https://github.com/apache/trafficserver/pull/1201
TS-4994: make state configuration in ts_lua plugin
@jpeach pls take a look when you have time.
You can merge this pull request into a Git repository by running:
$ git pull https
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1183
[approve ci]
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1183
yeah... just one extra empty line that clang-format does not like.
---
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
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1127
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1182
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1183
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1127
thanks. will squash before merge.
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1158
After the summit, here are the things I learn I should do
1) Add a flag to allow backward compatibility.
2) combine volume.config and hosting.config as well in this new lua
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1127
@jpeach, I have updated the PR according to the comments. Please take a
look.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1158
[approve ci]
---
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
GitHub user shukitchan opened a pull request:
https://github.com/apache/trafficserver/pull/1158
TS-5015: Convert storage configuration file to Lua
documentation is TBD. Also I want to clean up a bit since it is messy.
@jpeach, pls take a look and review.
You can merge
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1127
@jpeach pls take a look if you get a chance. thanks.
---
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
GitHub user shukitchan opened a pull request:
https://github.com/apache/trafficserver/pull/1127
[TS-4990] support new apis in ts_lua
support new apis in ts_lua
@jpeach pls take a look and review. Thanks.
You can merge this pull request into a Git repository by running
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1122
ð
looks good. data_ptr is simply (and redundantly) for reporting error. And
we don't actually need it.
---
If your project is set up for it, you can reply to this email
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1089
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1089
@zwoop & @jpeach , it is updated according to the comment. Please review
again. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply ap
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1089
@zwoop it should be good now. Please take a look.
And of course I will squash before merge.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1089
[approve ci]
---
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
GitHub user shukitchan opened a pull request:
https://github.com/apache/trafficserver/pull/1089
TS-4592: Add support for process ID and txn ID in ts_lua
@zwoop please take a look and review. Thanks.
You can merge this pull request into a Git repository by running:
$ git pull
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1064
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1064
ð
---
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
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/1065
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1065
pls state in the jira that it need a backport instead.
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1064
we should probably close #1065
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1064
the change 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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1064
[ci approve]
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1064
i think back port is done differently.
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1065
[approve ci]
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/1065
it looks fine.
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/906
no worries. I fixed that already.
---
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
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/906
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/906
i think it looks good. will try it out tonight before merging
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/859
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 pu
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/889
ð
---
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
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/859
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
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/859#discussion_r75625639
--- Diff: proxy/InkAPI.cc ---
@@ -2208,6 +2208,20 @@ TSUrlPathSet(TSMBuffer bufp, TSMLoc obj, const char
*value, int length)
return
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/776
@zwoop, @jacksontj and I have a short conversation a couple weeks ago. It
seemed like @jacksontj and his teammates are not pursuing the brotli works for
now.
So @bgaff, can we
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/408
@SolidWallOfCode , can you also take a look pls?
We are proposing a unit test framework for cpp api plugins. We have used
the webp transform plugin and updated it with unit tests
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/884#discussion_r75608137
--- Diff: doc/admin-guide/files/records.config.en.rst ---
@@ -2651,7 +2651,7 @@ Logging Configuration
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/859
oh and you need to subscribe to the dev@ mailing list to get those emails.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/859
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/
---
If your project is set
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/859
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.
---
If your project is set up for it, you can
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/851
ð
Good one. I can now have an string array of host names. Loop through them
and create log object for each of them that is slightly different from one
another.
---
If your
1 - 100 of 118 matches
Mail list logo
